Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Port limit attributes to the new attribute parsing infrastructure #145819

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

Open
jdonszelmann wants to merge 5 commits into rust-lang:master
base: master
Choose a base branch
Loading
from jdonszelmann:convert-limits

Conversation

Copy link
Contributor

@jdonszelmann jdonszelmann commented Aug 24, 2025

Doesn't pass tests, to be rebased on #145792 which will solve that

r? @fmease

Copy link
Collaborator

rustbot commented Aug 24, 2025

fmease is not on the review rotation at the moment.
They may take a while to respond.

@rustbot rustbot added A-attributes Area: Attributes (`#[...]`, `#![...]`) A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 24, 2025
@jdonszelmann jdonszelmann changed the title (削除) Convert limits (削除ここまで) (追記) Port limit attributes to the new attribute parsing infrastructure (追記ここまで) Aug 24, 2025

This comment has been minimized.

Copy link
Contributor

@jdonszelmann #145792 is mostly ready so you can probably rebase now :3

This comment was marked as resolved.

@jdonszelmann jdonszelmann marked this pull request as ready for review August 28, 2025 10:27
@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 Aug 28, 2025
Copy link
Collaborator

rustbot commented Aug 28, 2025

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

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.

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

This comment has been minimized.

Copy link
Contributor Author

does conflict with #145937 but it's an easy rebase so we'll just see what merges first
fmease reacted with thumbs up emoji

@@ -6,15 +6,5 @@ LL | #![recursion_limit = "-100"]
| |
| not a valid integer

error: `limit` must be a non-negative integer
--> $DIR/invalid_digit.rs:3:1
|
Copy link
Contributor Author

@jdonszelmann jdonszelmann Aug 28, 2025

Choose a reason for hiding this comment

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

this doubled up error cannot be prevented. If we do, we'd get a similar situation as with crate_name where we either forget some cases, or delay as bug giving an ice when this literal happens to be a macro that before expansion is invalid but expanded is valid. e.g. concat!("100") which before expansion is an expression we should reject but after expansion is a perfectly valid integer literal

Copy link
Member

Choose a reason for hiding this comment

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

It's not doubled up? Your PR removes duplication looking at the diff. Am I missing something?

Copy link
Member

fmease commented Aug 28, 2025

I presume this fixes some of these reported ICEs: #145922? You don't need to add regression tests, not worth it I think.

Copy link
Collaborator

bors commented Aug 28, 2025

☔ The latest upstream changes (presumably #145958) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Contributor Author

I think the ices are already mostly fixed because we don't delay bugs anymore for this specific case. So I think it's fixed without this pr but we'll want this anyway

Copy link
Collaborator

rustbot commented Aug 29, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Copy link
Contributor Author

@rustbot ready

Copy link
Member

@fmease fmease left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

@@ -1245,7 +1246,8 @@ pub fn get_crate_name(sess: &Session, krate_attrs: &[ast::Attribute]) -> Symbol
// in all code paths that require the crate name very early on, namely before
// macro expansion.

let attr_crate_name = parse_crate_name(sess, krate_attrs, ShouldEmit::EarlyFatal);
let attr_crate_name =
parse_crate_name(sess, krate_attrs, ShouldEmit::EarlyFatal { also_emit_lints: true });
Copy link
Member

Choose a reason for hiding this comment

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

Ah, cool! You've (added &) set also_emit_lints to true for warning on e.g., duplicate #![crate_names], is that correct?

I've experimented a bit and noticed that printf '#![crate_name = "x"]#![crate_name="x"]' | rustc +master-2025年08月29日 - --print=crate-name doesn't emit any warning on master. Does your change incidentally fix that? (It does emit during normal execution)

Copy link
Contributor Author

@jdonszelmann jdonszelmann Aug 31, 2025

Choose a reason for hiding this comment

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

it doesn't fix cause here we never get to the point where we emit buffered lints I think hehe

fmease reacted with thumbs up emoji
Copy link
Contributor Author

@jdonszelmann jdonszelmann Aug 31, 2025

Choose a reason for hiding this comment

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

early lints are emitted and buffered but they never appear

LL | #[recursion_limit="0200"]
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: This attribute does not have an `!`, which means it is applied to this module
Copy link
Member

@fmease fmease Aug 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

(preexisting) The This should be lower-case, https://rustc-dev-guide.rust-lang.org/diagnostics.html#diagnostic-output-style-guide, can be fixed in a different PR.

Copy link
Contributor Author

@jdonszelmann jdonszelmann Aug 31, 2025

Choose a reason for hiding this comment

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

Copy link
Contributor

@scrabsha scrabsha Sep 1, 2025

Choose a reason for hiding this comment

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

Oops, sorry. I'll open a PR today (ish)

Copy link
Contributor

@scrabsha scrabsha Sep 1, 2025

Choose a reason for hiding this comment

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

Ok, i opened #146080.

@@ -6,15 +6,5 @@ LL | #![recursion_limit = "-100"]
| |
| not a valid integer

error: `limit` must be a non-negative integer
--> $DIR/invalid_digit.rs:3:1
|
Copy link
Member

Choose a reason for hiding this comment

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

It's not doubled up? Your PR removes duplication looking at the diff. Am I missing something?

@fmease fmease 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 Aug 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmease fmease fmease approved these changes

+1 more reviewer

@scrabsha scrabsha scrabsha left review comments

Reviewers whose approvals may not affect merge requirements
Labels
A-attributes Area: Attributes (`#[...]`, `#![...]`) A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /