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.

Sign up for GitHub

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

Jump to bottom

Visit more targets when validating attributes #80920

Merged
merged 2 commits into from
Feb 15, 2021
Merged

Visit more targets when validating attributes #80920

merged 2 commits into from
Feb 15, 2021

Conversation

rylev
Copy link
Member

@rylev rylev commented Jan 11, 2021

This begins to address #80048, allowing for additional validation of attributes.

There are more refactorings that can be done, though I think they should be tackled in additional PRs:

  • ICE when a builtin attribute is encountered that is not checked
  • Move some of the attr checking done ast_validation into rustc_passes
    • note that this requires a bit of additional refactoring, especially of extern items which currently parse attributes (and thus are a part of the AST) but do not possess attributes in their HIR representation.
  • Rename Target to AttributeTarget
  • Refactor attribute validation completely to go through Visitor::visit_attribute.
    • This would require at a minimum passing Target into this method which might be too big of a refactoring to be worth it.
    • It's also likely not possible to do all the validation this way as some validation requires knowing what other attributes a target has.

r? @davidtwco

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

nagisa commented Jan 11, 2021

@bors r+ LGTM!

@bors
Copy link
Contributor

bors commented Jan 11, 2021

📌 Commit 39a1d774fcbfbb3dcf434cdf8ad9e0b97c3ecc1d has been approved by nagisa

@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 Jan 11, 2021
@davidtwco
Copy link
Member

In previous PRs which have checked more targets when validating attributes (#77015 and #79073), we did a crater run to check for regressions - this is technically a breaking change. Might be worth doing that here too.

@nagisa
Copy link
Member

nagisa commented Jan 11, 2021

@bors r- try

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 11, 2021
@bors
Copy link
Contributor

bors commented Jan 11, 2021

⌛ Trying commit 39a1d774fcbfbb3dcf434cdf8ad9e0b97c3ecc1d with merge bb9dbf1d07454a99066eabaabd5819851281494b...

@bors
Copy link
Contributor

bors commented Jan 11, 2021

☀️ Try build successful - checks-actions
Build commit: bb9dbf1d07454a99066eabaabd5819851281494b (bb9dbf1d07454a99066eabaabd5819851281494b)

@nagisa

This comment has been minimized.

Sign in to view
@craterbot

This comment has been minimized.

Sign in to view
@nagisa
Copy link
Member

nagisa commented Jan 11, 2021

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-80920 created and queued.
🤖 Automatically detected try build bb9dbf1d07454a99066eabaabd5819851281494b
⚠️ Try build based on commit 6526e5c, but latest commit is 39a1d774fcbfbb3dcf434cdf8ad9e0b97c3ecc1d. Did you forget to make a new try build?
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 11, 2021
@ehuss
Copy link
Contributor

ehuss commented Jan 11, 2021

Just wanted to give a heads up, I think PR #80641 is doing something almost identical, you may want to coordinate, and at least make sure the same tests are included and the issues (like "inline") are covered.

@rylev rylev mentioned this pull request Jan 12, 2021
Merged
@craterbot
Copy link
Collaborator

🚧 Experiment pr-80920 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@ehuss ehuss mentioned this pull request Jan 13, 2021
Open
6 tasks
@rylev
Copy link
Member Author

rylev commented Jan 14, 2021

Here's an issue on #[inline] on associated constants that we might want to check we fix with this.

@craterbot
Copy link
Collaborator

🎉 Experiment pr-80920 is completed!
📊 19 regressed and 8 fixed (140061 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jan 16, 2021
@davidtwco
Copy link
Member

Looks like this regresses olm (three dependencies, now also failing) and RustWebServer (no dependencies).

@davidtwco davidtwco 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 Jan 18, 2021
@davidtwco
Copy link
Member

I'll nominate this for the language team to decide if the breaking changes are acceptable.

@davidtwco davidtwco added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 25, 2021
@sanxiyn
Copy link
Member

sanxiyn commented Jan 31, 2021

Note that #[used] provides access to llvm.used, but support in Rust is known to be incomplete. I think, at least, #68322 should be fixed.

@nikomatsakis
Copy link
Contributor

cc @jhaye and @poljar -- the regression on olm-rs seems real and affects the most recent version. Are you able to issue a 1.0.1 release that removes the #[used] from being used on fields?

@Mark-Simulacrum
Copy link
Member

We discussed this during lang meeting today and concluded that we can move forward (we want to ping authors of the affected crates, but I believe have done so in Niko's comment and @joshtriplett is going to file an issue on the other project).

We also noted that generally speaking this breakage is unfortunate and we may want to move to only breaking crates over edition boundaries in cases like this, but we're not prepared to reconsider the current pre-existing behavior here for inline in other contexts. It's unclear to me that we get a huge amount of benefit from avoiding the breakage in cases like this too.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed I-nominated S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Feb 2, 2021
@poljar
Copy link

poljar commented Feb 2, 2021

cc @jhaye and @poljar -- the regression on olm-rs seems real and affects the most recent version. Are you able to issue a 1.0.1 release that removes the #[used] from being used on fields?

Sure, PR is up. Sorry for the inconvenience.

@jhaye
Copy link

jhaye commented Feb 2, 2021

cc @jhaye and @poljar -- the regression on olm-rs seems real and affects the most recent version. Are you able to issue a 1.0.1 release that removes the #[used] from being used on fields?

Sure, PR is up. Sorry for the inconvenience.

v1.0.1 was just published.

@davidtwco
Copy link
Member

I think we can probably land this then, could you rebase @rylev?

@rylev rylev force-pushed the check_attr-refactor branch from 39a1d77 to 396022b Compare February 9, 2021 20:55
@rylev
Copy link
Member Author

rylev commented Feb 9, 2021

@davidtwco I've rebased. The PR is a lot less exciting than it started as, but there might be a reason to still merge it. Perhaps though we should further refactor how attributes are validated in other parts of the compiler.

@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 13, 2021

📌 Commit 9f0e1d4 has been approved by davidtwco

@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 Feb 13, 2021
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Feb 14, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 14, 2021
@bors
Auto merge of rust-lang#82103 - Dylan-DPC:rollup-5wv8rid, r=Dylan-DPC
9503ea1 
Rollup of 11 pull requests

Successful merges:

 - rust-lang#80523 (#[doc(inline)] sym_generated)
 - rust-lang#80920 (Visit more targets when validating attributes)
 - rust-lang#81720 (Updated smallvec version due to RUSTSEC-2021-0003)
 - rust-lang#81891 ([rustdoc-json] Make `header` a vec of modifiers, and FunctionPointer consistent)
 - rust-lang#81912 (Implement the precise analysis pass for lint `disjoint_capture_drop_reorder`)
 - rust-lang#81914 (Fixing bad suggestion for `_` in `const` type when a function rust-lang#81885)
 - rust-lang#81919 (BTreeMap: fix internal comments)
 - rust-lang#81927 (Add a regression test for rust-lang#32498)
 - rust-lang#81965 (Fix MIR pretty printer for non-local DefIds)
 - rust-lang#82029 (Use debug log level for developer oriented logs)
 - rust-lang#82056 (fix ice (rust-lang#82032))

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ac1d26b into rust-lang:master Feb 15, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 15, 2021
@eddyb
Copy link
Member

eddyb commented Feb 25, 2021

Sorry I missed these parts of the description, but hopefully this helps for future PRs:

This would require at a minimum passing Target into this method which might be too big of a refactoring to be worth it.

You can always keep data in the visitor itself, but that's harder to make sure nothing goes wrong with.

It's also likely not possible to do all the validation this way as some validation requires knowing what other attributes a target has.

It's a bit weird IMO that visit_attributes doesn't exist. That might be a good way to do it.

@eddyb eddyb mentioned this pull request Mar 2, 2021
Closed
@steffahn steffahn mentioned this pull request Jun 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidtwco davidtwco davidtwco approved these changes

Assignees

@davidtwco davidtwco

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.52.0
Development

Successfully merging this pull request may close these issues.