Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update BARE_TRAIT_OBJECT and ELLIPSIS_INCLUSIVE_RANGE_PATTERNS to errors in Rust 2021 #83213
Update BARE_TRAIT_OBJECT and ELLIPSIS_INCLUSIVE_RANGE_PATTERNS to errors in Rust 2021 #83213
Changes from all commits
c2d0f14
1d84947
152c862
4f67392
2c1429c
a2b1347
d7b2263
cebc520
aa4457f
43f9d0a
cd8392d
4e25ae4
4193b0b
7a50392
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think you still need to specify the edition here (as least to be coherent with the failing code example).
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.
Note: lint passes typically do not have side effects, if a lint is disabled there should ideally be no behavior differences between running the pass or not (and that's a potential optimization to do in the future).
I'm not sure how strictly the compiler team follows this; it's possible that when we do such optimizations we'll have to go back and audit our lints (which is fine!), but I just want to flag this.
I'm not sure how str
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 only concern is that performing this check in the appropriate place as if we had no editions could cause divergence in the behavior. Emitting the error in the same place as we currently emit the lint should make the transition less painful than if we start denying in places where we currently don't even lint. That being said, this could be accepted as acceptable breakage to bestow on the ecosystem in the name of codebase cleanliness.
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.
Hmm, I missed this discussion the first time around. A few thoughts:
fn required(&self) -> bool { false }
to the trait, and this lint could return true. This might be convenient if we commonly wind up with things that are errors in one edition and lints in another.Moving the logic out from the "lint helper" just seems like a bit of extra work, I'm not sure what would be a nice place to put it, we'd have to reproduce the "traverse the tree" code. That said, I don't object to it.
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.
@estebank I don't quite understand how behavior would change -- do you just mean altering the order of errors that get emitted?
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.
I'm also not quite following. That said, I don't think we hold ourselves to a sufficiently high standard here today that I would be comfortable applying a "no pass" optimization if lints were disabled. We also merge all built-in lint passes into a few groups (by when they need to run), so there's not really an actual "per-lint" full traversal of any tree, so it's not obvious a major win is to be had. (But I don't think we've profiled much).
I think this seems OK for now.
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.
As long as we have tests for the hard error, I'm comfortable with it.
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.
I do wish we could make this more unified. I feel like the
struct_span_lint_hir
ought to check the edition info and convert it to a hard error automatically. Probably better to just open an issue for this and leave it for future cleanup.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.
Filed #84625