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

Diagnostic namespace #111780

Merged
merged 1 commit into from
Jul 28, 2023
Merged

Diagnostic namespace #111780

merged 1 commit into from
Jul 28, 2023

Conversation

weiznich
Copy link
Contributor

This PR implements the basic infrastructure for accepting the #[diagnostic] attribute tool namespace as specified in rust-lang/rfcs#3368. Note: This RFC is not merged yet, but it seems like it will be accepted soon. I open this PR early on to get feedback on the actual implementation as soon as possible. This hopefully enables getting at least the diagnostic namespace to stable rust "soon", so that crates do not need to bump their MSRV if we stabilize actual attributes in this namespace.

This PR only adds infrastructure accept attributes from this namespace, it does not add any specific attribute. Therefore the compiler will emit a lint warning for each attribute that's actually used. This namespace is added behind a feature flag, so it will be only available on a nightly compiler for now.

cc @estebank as they've supported me in planing, specifying and implementing this feature.

@rustbot
Copy link
Collaborator

rustbot commented May 20, 2023

r? @b-naber

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 20, 2023
@rustbot
Copy link
Collaborator

rustbot commented May 20, 2023

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@matthewjasper
Copy link
Contributor

cc @petrochenkov

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@weiznich weiznich mentioned this pull request May 26, 2023
Closed
3 tasks
@petrochenkov petrochenkov self-assigned this May 26, 2023
@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 May 27, 2023
@weiznich weiznich force-pushed the diagnostic_namespace branch 2 times, most recently from d48d0d3 to 2408bc6 Compare June 2, 2023 06:52
@weiznich
Copy link
Contributor Author

weiznich commented Jun 2, 2023

I've rebased this on top of #112086 to resolve @petrochenkov concerns. Merging this PR needs to wait till #112086 has landed.

@Noratrieb Noratrieb mentioned this pull request Jun 3, 2023
Merged
@petrochenkov petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 3, 2023
@weiznich
Copy link
Contributor Author

weiznich commented Jun 8, 2023

To just leave that as comment here: I'm on vacation for the next weeks, so I likely won't be able to update the PR for potential conflicts in that time. Hopefully the name resolution fix is merged when I'm back. If necessary I will fix conflicts after I'm back

@bors

This comment was marked as resolved.

Sign in to view
@weiznich
Copy link
Contributor Author

Let's move all that stuff (both feature gate and linting) back to fn smart_resolve_macro_path, my apologies.

I've removed the relevant changes for the linting from the PR, but I left the feature gating in place as it's just a local addition to the feature gate pass and it seems to fit there quite well. If that's wrong I will happily move it back as well, but I assume the main problem with the other change was the addition of the NodeId parameter to the relevant ast-pass function. As that's not required for the feature gate it might be fine to keep it there?

@rustbot reviewer

@rustbot rustbot 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 Jul 27, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
estebank
estebank reviewed Jul 27, 2023
View reviewed changes
tests/ui/diagnostic_namespace/requires_path.stderr
Comment on lines +1 to +5
error: cannot find attribute `diagnostic` in this scope
--> $DIR/requires_path.rs:3:3
|
LL | #[diagnostic]
| ^^^^^^^^^^
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before stabilization of the namespace, we'll want to customize this error to be something along the lines of "#[diagnostic] needs to be followed by one of *".

petrochenkov
petrochenkov reviewed Jul 27, 2023
View reviewed changes
compiler/rustc_resolve/src/macros.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/macros.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

I've removed the relevant changes for the linting from the PR, but I left the feature gating in place as it's just a local addition to the feature gate pass and it seems to fit there quite well.

Yes, that's fine too.

@rustbot author

@rustbot rustbot 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 Jul 27, 2023
@petrochenkov
Copy link
Contributor

r=me after addressing #111780 (comment) and squashing commits.

@weiznich @petrochenkov
Introduce the #[diagnostic] attribute namespace
5b57666 
Co-authored-by: est31 <est31@users.noreply.github.com>

Co-authored-by: Esteban Kuber <estebank@users.noreply.github.com>

Co-authored-by: Vadim Petrochenkov <vadim.petrochenkov@gmail.com>
@petrochenkov
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Jul 28, 2023

📌 Commit 5b57666 has been approved by petrochenkov

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 28, 2023
@bors
Copy link
Contributor

bors commented Jul 28, 2023

⌛ Testing commit 5b57666 with merge 317ec04...

@bors
Copy link
Contributor

bors commented Jul 28, 2023

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 317ec04 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 28, 2023
@bors bors merged commit 317ec04 into rust-lang:master Jul 28, 2023
11 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Jul 28, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (317ec04): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.6% [2.6%, 2.6%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -1.9% [-3.0%, -0.8%] 4
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.6% [-3.0%, -2.3%] 2
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 650.875s -> 650.027s (-0.13%)

@estebank estebank mentioned this pull request Aug 3, 2023
Open
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@est31 est31 est31 left review comments

@petrochenkov petrochenkov petrochenkov left review comments

@estebank estebank estebank approved these changes

Assignees

@petrochenkov petrochenkov

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc 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. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.73.0
Development

Successfully merging this pull request may close these issues.
10 participants
@weiznich @rustbot @rust-log-analyzer @matthewjasper @bors @petrochenkov @rust-timer @estebank @est31 @b-naber