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

Pass real crate-level attributes to pre_expansion_lint #89214

Merged
merged 2 commits into from
Sep 27, 2021

Conversation

smoelius
Copy link
Contributor

@smoelius smoelius commented Sep 24, 2021

The PR concerns the unstable feature register_tool (#66079).

The feature's implementation requires the attributes of the crate being compiled, so that when attributes like allow(foo::bar) are encountered, it can be verified that register_tool(foo) appears in the crate root.

However, the crate's attributes are not readily available during early lint passes. Specifically, on this line, krate.attrs appears to be the attributes of the current source file, not the attributes of the whole crate:

builder: LintLevelsBuilder::new(sess, warn_about_weird_lints, lint_store, &krate.attrs),

Consequently, "unknown tool" errors were being produced when allow(foo::bar) appeared in a submodule, even though register_tool(foo) appeared in the crate root.

EDITED: The proposed fix is to obtain the real crate-level attributes in configure_and_expand and pass them to pre_expansion_lint. (See @petrochenkov's comment below.)

The original "prosed fix" text follows.


The proposed fix is to add an error_on_unknown_tool flag to LintLevelsBuilder. The flag controls whether "unknown tool" errors are emitted. The flag is set during late passes, but not earlier.

More specifically, this PR contains two commits:

  • The first adds a known-tool-in-submodule UI test that does not currently pass.
  • The second adds the error_on_unknown_tool flag. The new test passes with the addition of this flag.

This change has the added benefit of eliminating some errors that were duplicated in existing tests.

To the reviewer: please check that I implemented the UI test correctly.

The test currently fails. The next commit fixes it.
@rust-highfive
Copy link
Collaborator

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 24, 2021
@nagisa
Copy link
Member

nagisa commented Sep 24, 2021

r? @petrochenkov

@rust-highfive rust-highfive assigned petrochenkov and unassigned nagisa Sep 24, 2021
@petrochenkov
Copy link
Contributor

krate.attrs appears to be the attributes of the current source file, not the attributes of the whole crate

This is a bug and a regression from #69838.
pre_expansion_lint is called after loading any new module, and that module is passed to it after being artificially turned into ast::Crate, just because pre_expansion_lint works with ast::Crate.
Real crate-level attributes are available at that point and just need to be additionally passed to pre_expansion_lint.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 24, 2021
@smoelius
Copy link
Contributor Author

Thanks, @petrochenkov. Please leave the PR open for now while I try to find a proper fix.

@smoelius
Copy link
Contributor Author

@petrochenkov Is this about what you had in mind?

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 27, 2021
@petrochenkov
Copy link
Contributor

Yes, this is exactly what I meant.
@bors r+

@smoelius Could you also update the PR description before the merge? (It's outdated, but I'll be included into the merge commit.)

@bors
Copy link
Contributor

bors commented Sep 27, 2021

📌 Commit 1e15bbe has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 27, 2021
@smoelius smoelius changed the title Add error_on_unknown_tool flag to LintLevelsBuilder Pass real crate-level attributes to pre_expansion_lint Sep 27, 2021
@smoelius
Copy link
Contributor Author

@smoelius Could you also update the PR description before the merge? (It's outdated, but I'll be included into the merge commit.)

Absolutely. Please let me know if what I wrote is insufficient.

@bors
Copy link
Contributor

bors commented Sep 27, 2021

⌛ Testing commit 1e15bbe with merge 98c8619...

@bors
Copy link
Contributor

bors commented Sep 27, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 98c8619 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 27, 2021
@bors bors merged commit 98c8619 into rust-lang:master Sep 27, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 27, 2021
@smoelius smoelius deleted the register_tool branch September 27, 2021 21:31
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (98c8619): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants