skip dyn keyword lint under macros #59463
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The existing `KeywordIdents` lint blindly scans the token stream for a macro or macro definition. It does not attempt to parse the input, which means it cannot distinguish between occurrences of `dyn` that are truly instances of it as an identifier (e.g. `let dyn = 3;`) versus occurrences that follow its usage as a contextual keyword (e.g. the type `Box<dyn Trait>`). In an ideal world the lint would parse the token stream in order to distinguish such occurrences; but in general we cannot do this, because a macro_rules definition does not specify what parsing contexts the macro being defined is allowed to be used within. So rather than put a lot of work into attempting to come up with a more precise but still incomplete solution, I am just taking the short cut of not linting any instance of `dyn` under a macro. This prevents `rustfix` from injecting bugs into legal 2015 edition code.
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
Centril
reviewed
Mar 27, 2019
…ed as keyword in test. Back-story: After reflection this morning, I realized that the previous form of this test would allow the macro invocation to treat the `dyn` input as a raw-identifier rather than a keyword, and since the input was discarded by that version of the macro, the test would pass despite the detail that the input `dyn` should not have been parsed as a raw-identifier. This revision fixes that oversight, by actually *using* the macro input to construct a `Box<dyn Trait>` type.
Review feedback asked for the test to be generalized to include macros 2.0; that generalization is dyn-2015-idents-in-decl-macros-unlinted.rs As a drive-by, I also decided to revise the test to make it clear *why* we cannot generally lint these cases. (I already had similar demonstrations in dyn-2015-edition-keyword-ident-lint.rs, but it does not hurt to try to emphasize matters.) I also added some commentary on the cases where we could choose to make the lint smarter, namely the situations where a macro is *definitely* using `dyn` as an identifier (because it is using it as a path component).
|
@bors r+ |
|
📌 Commit 528366d has been approved by |
bors
added
S-waiting-on-bors
Centril
added a commit
to Centril/rust
that referenced
this pull request
Mar 30, 2019
…rd-lint-under-macros, r=matthewjasper skip dyn keyword lint under macros This PR is following my own intuition that `rustfix` should never inject bugs into working code (even if that comes at the expense of it failing to fix things that will become bugs). Fix rust-lang#56327
bors
added a commit
that referenced
this pull request
Mar 30, 2019
Rollup of 5 pull requests Successful merges: - #59343 (rustc(codegen): uncache `def_symbol_name` prefix from `symbol_name`.) - #59380 (Fix invalid DWARF for enums when using ThinLTO) - #59463 (skip dyn keyword lint under macros) - #59539 (Fix infinite recursion) - #59544 (manifest: only include miri on the nightly channel) Failed merges: r? @ghost
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
This PR is following my own intuition that
rustfixshould never inject bugs into working code (even if that comes at the expense of it failing to fix things that will become bugs).Fix #56327