-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Remove impl Foo for .. {}
in favor auto trait Foo {}
#46480
Conversation
Got error on rustdoc's error index test, it's not parsing |
@leodasvacas Try to add a https://play.rust-lang.org/?gist=5e9637830e8e6cb05b3f48a8e7bc5cf7&version=nightly |
@kennytm Nailed it as usual, fixed it. |
src/libsyntax/parse/parser.rs
Outdated
// Our goal here is to parse an arbitrary path `a::b::c` but not something that starts | ||
// like a path (1 token), but it fact not a path. | ||
// `union::b::c` - path, `union U { ... }` - not a path. | ||
// `crate::b::c` - path, `crate struct S;` - not a path. | ||
} else if self.token.is_path_start() && | ||
!self.token.is_qpath_start() && | ||
!self.is_union_item() && | ||
!self.is_crate_vis() { | ||
!self.is_crate_vis() && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
src/libsyntax/parse/parser.rs
Outdated
(self.token.is_keyword(keywords::Auto) | ||
&& self.look_ahead(1, |t| t.is_keyword(keywords::Trait))) | ||
|| // unsafe auto trait | ||
(self.token.is_keyword(keywords::Unsafe) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace
[00:03:11] tidy error: /checkout/src/libsyntax/parse/parser.rs:3946: trailing whitespace
[00:03:11] tidy error: /checkout/src/libsyntax/parse/parser.rs:4060: trailing whitespace
a66ff70
to
f252157
Compare
self.err_handler().span_err(item.span, | ||
"auto traits cannot have generics"); | ||
struct_span_err!(self.session, item.span, E0567, | ||
"Auto traits cannot have type parameters").emit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Auto" -> "auto", error messages are not normally capitalized.
Also "type parameters" -> "generic parameters" and the condition a few lines above is incorrect, it should use !generics.is_parameterized()
, because lifetime parameters are disallowed as well (they are currently rejected on impls for ..
).
self.err_handler().span_err(item.span, | ||
"auto traits cannot have super traits"); | ||
struct_span_err!(self.session, item.span, E0568, | ||
"Auto traits cannot have predicates").emit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message was better before, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I use the new error code and the old error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, why not.
E0533, // `{}` does not name a unit variant, unit struct or a constant | ||
// E0563, // cannot determine a type for this `impl Trait`: {} // removed in 6383de15 | ||
E0564, // only named lifetimes are allowed in `impl Trait`, | ||
// but `{}` was found in the type `{}` | ||
E0567, // auto traits can not have type parameters | ||
E0568, // auto-traits can not have predicates, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you comment out the obsoleted error codes instead of removing them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tidy is merciless, it gives me duplicate error codes even if it's commented out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right, they are not removed, just moved to AST validation.
src/libsyntax/parse/parser.rs
Outdated
@@ -6167,7 +6150,8 @@ impl<'a> Parser<'a> { | |||
let is_auto = if self.eat_keyword(keywords::Trait) { | |||
IsAuto::No | |||
} else { | |||
self.eat_auto_trait(); | |||
self.eat_keyword(keywords::Auto); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rule of thumb: eat_keyword
shouldn't be used unless you use the returned result.
expect_keyword
or simply bump
would be better here.
LGTM, modulo comments. The only question is - is it too soon to remove this or not? |
I general we only deprecate or remove stuff once the replacement has reached the stable channel. If a project (even using unstable features) builds without warnings on all three release channels, it should be possible to keep it that way. |
05dd1a9
to
52070a1
Compare
☔ The latest upstream changes (presumably #46532) made this pull request unmergeable. Please resolve the merge conflicts. |
52070a1
to
35b18a0
Compare
☔ The latest upstream changes (presumably #45047) made this pull request unmergeable. Please resolve the merge conflicts. |
35b18a0
to
02fcc0a
Compare
☔ The latest upstream changes (presumably #45525) made this pull request unmergeable. Please resolve the merge conflicts. |
Can you say a bit more about this? Are you imagining that e.g. the unstable syntax is removed with |
Uh, what I wrote doesn’t make sense. A project that uses unstable features cannot build on beta or stable. I was thinking of deprecating stable features. Never mind me :) |
02fcc0a
to
6c2e1a6
Compare
☔ The latest upstream changes (presumably #46754) made this pull request unmergeable. Please resolve the merge conflicts. |
You can blame crossbeam if you want (I wouldn't, this particular breakage is only obvious in hindsight -- perfectly competent programmers could have missed it, and indeed did). That doesn't change the fact that countless transitive dependencies of crossbeam would stop compiling on stable for no good reason -- there's no soundness bug and no gross negligence on crossbeam's part, and preventing the breakage isn't even difficult AFAICT. Breaking all this code and forcing it to update would be user-hostile IMO. |
@rkruppe What about splitting the deprecation so that it functionally stops working, but it still parses (and emits some kind of warning?) |
@bluss That's pretty much what I suggested earlier, just without the warning. I don't see a big need for a warning -- it can just error if the old syntax remains after cfg stripping, and be silently stripped otherwise. But that's a minor difference. The important part is that the old syntax still parses but emits an error soon after cfg stripping. |
Ha, this is an interesting issue. #[cfg(nightly)]
old_syntax In this case we need to keep minimal support for any old syntax in the parser and AST (e.g. However, the larger effect seems pretty scary - we can't experiment with any new features in rustc if they require parser support because we'll may have to keep this support forever. |
Lazily parsing item bodies would mitigate this to some extent right? #[cfg(unstable)]
mod future {
~wacky FUTURE _syntax!
impl RemovedSyntax for .. {}
} If the braced module body is parsed after cfg stripping, this should be okay. We already support this because it is what happens for out-of-line modules |
Yeah, quite unfortunate. There is one mitigating factor, though: Any such features introduced now or in the future will have the same problem (parser error even if cfg'd out) on all previous releases. Therefore, it's reasonably possible that a crate author that makes the same mistake the crossbeam authors made will get bug reports from users on old stable releases, alerting them of the problem. We could declare such changes "acceptable breakage" and start recommending crates use macros for conditional use of nightly-only syntax features. If enough crates follow this guideline (aided by the mitigating factor described above), or the feature was never in widespread use anyway, we could sometimes get away with just removing parser support. But crossbeam is big enough that we really shouldn't break it even if that had been long-standing policy. It's still possible that any given feature winds up in a popular crate that cfg-gated it improperly, but it hopefully limits the amount of obsolete syntax the parser needs to accumulate support for. |
@dtolnay |
I agree the current impact of breakage here is not acceptable. I'll update this patch to have the minimum necessary support for the deprecated syntax. @petrochenkov is it viable to support this only in the parser but not in the AST? |
@leodasvacas
The |
☔ The latest upstream changes (presumably #47156) made this pull request unmergeable. Please resolve the merge conflicts. |
@petrochenkov Alright, do we need it in HIR also, or can we error if we encounter it in lowering? When the new versions of crossbeam get a sufficiently large share of the daily downloads in crates.io, perhaps we may reconsider completely dropping support for the old syntax. |
@leodasvacas
No, no HIR, no pretty-printing support or anything, the single purpose of it is to report an error at some point (in AST validation or during lowering to HIR), after that it's no longer necessary. |
@leodasvacas |
@leodasvacas |
Sorry for being slow here, I'll have time next week, if you want to do it
sooner you're welcome!
Em 13/01/2018 12:18 PM, "Vadim Petrochenkov" <[email protected]>
escreveu:
… @leodasvacas <https://github.com/leodasvacas>
I'd like to land this PR sooner.
I can rebase it and finish the parsing stuff if you don't have time.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#46480 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AJbXdpp0KMoSMjpm9U_k_MgTTYCcjTBJks5tKLs1gaJpZM4Qz7Pc>
.
|
Closing in favor of #47416 |
…henkov Remove `impl Foo for .. {}` in favor `auto trait Foo {}` Rebase of #46480 with restored parsing support.
The new syntax was introduced and the old one deprecated in #45247. This removes the old syntax and simplifies some code as a result. Some errors that were possible with the old syntax no longer make sense. WF checking is moved to an ast validation.
There is some conflict with #46455, nothing serious. I guess this PR should be preferred where there is a conflict.