Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unseal Spanned again #1524

Closed
wants to merge 1 commit into from
Closed

Unseal Spanned again #1524

wants to merge 1 commit into from

Conversation

ijackson
Copy link

Spanned was not sealed in syn 1.

Currently in syn 2 it is nominally sealed, but since quote::ToTokens isn't sealed, Spanned can be implemented for any foreign type, provided one is willing to provide a (possibly broken) implemntation of ToTokens.

This means that sealing Spanned has no semver advantage, and neither does it defend syn from any bad behaviour on the part of downstream implementors.

This is blocking at least one of my downstreams of syn from updating to syn 2.

See #1441

Spanned was not sealed in syn 1.

Currently in syn 2 it is nominally sealed, but since quote::ToTokens
isn't sealed, Spanned *can* be implemented for any foreign type,
provided one is willing to provide a (possibly broken) implemntation
of ToTokens.

This means that sealing Spanned has no semver advantage, and neither
does it defend syn from any bad behaviour on the part of downstream
implementors.

See dtolnay#1441
Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not interested in this until possibly after Span::join works on stable. You are correct that ToTokens is the supported way to implement Spanned. That means that syn::Error::new_spanned and other APIs can behave usefully on stable when a direct implementation of Spanned would not.

@dtolnay dtolnay closed this Oct 30, 2023
@ijackson ijackson mentioned this pull request Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants