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

Why is Spanned sealed? #1441

Closed
TedDriggs opened this issue Apr 6, 2023 · 6 comments
Closed

Why is Spanned sealed? #1441

TedDriggs opened this issue Apr 6, 2023 · 6 comments

Comments

@TedDriggs
Copy link
Contributor

I saw that syn 2.0 seals the Spanned trait (and that it was previously also sealed in #439). This is an issue for my crate darling, which has some structs that impl Spanned but don’t impl ToTokens.

I could come up with a workaround, such as creating darling::Spanned with a blanket impl to the syn one, but I’d like to understand why the trait was sealed so that my workaround isn’t somehow flawed/doomed to failure.

@ijackson
Copy link

ijackson commented Jul 9, 2023

Additionally, this doesn't seem to be documented in the release notes for syn 2, in the list of breaking changes.

I see that Spanned is still implemented for every quote::ToTokens. In syn 2 with the sealed Spanned trait, this has required:

impl<T: ?Sized + ToTokens> Sealed for T {}

Previously, in syn 1.0, this was achieved in babcfc6 "Use quote's Spanned to provide impl for Span". That commit unseals Spanned.

The situation with syn 2 is rather strange, now. Spanned isn't really sealed: you can always impl Spanned: all you have to do is impl ToTokens too, since ToTokens isn't sealed. But of course your type might not really be ToTokens so you might have to make your ToTokens impl broken (you could make it panic, for example).

(As an example of a type that could be Spanned but not ToTokens: anything that can be represented as tokens but only fallibly.)

I can't see any logical reason why the combination "Spanned but not ToTokens" is particularly hazardous.

@bzm3r
Copy link

bzm3r commented Jul 14, 2023

In general, I am finding something similar with a lot of the traits that are Sealed, or structs/functionality that is hidden as it is marked private.

What exactly is syn protecting us from? Is it a way to make the public API safer? As it is, I often end up re-implementing something that is similar to what I see in the syn source code.

ijackson added a commit to ijackson/rust-syn that referenced this issue Oct 30, 2023
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
@ijackson
Copy link

I proposed to unseal this in #1524. @dtolnay has rejected that MR. Accordingly, I think people who want Spanned for their own non-ToTokens types may need to invent a local Spanned trait, or something.

@dtolnay
Copy link
Owner

dtolnay commented Apr 17, 2024

That's right, using your own alternative trait is the way to go. For those use cases, bear in mind that a -> Span signature is not what you would want anyway.

@dtolnay dtolnay closed this as completed Apr 17, 2024
@TedDriggs
Copy link
Contributor Author

Why isn’t that signature what we want in most cases?

@dtolnay
Copy link
Owner

dtolnay commented Apr 17, 2024

I touched on this in the linked PR in #1524 (review). In stable Rust, a Span always refers to exactly 1 single token. So a function that returns Span is not useful for anything that intends to refer to a whole syntax tree node or multiple tokens. For example this is why syn::Error::new_spanned is not built on a trait that returns Span.

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

No branches or pull requests

4 participants