Skip to content

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.

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
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Port limit attributes to the new attribute parsing infrastructure #145819

wants to merge 5 commits into from

Conversation

jdonszelmann
Copy link
Contributor

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

r? @fmease

@rustbot
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
@jdonszelmann jdonszelmann mentioned this pull request Aug 24, 2025
Open
9 tasks
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@scrabsha
Copy link
Contributor

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

@bors

This comment was marked as resolved.

Sign in to view
@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
@rustbot
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

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jdonszelmann
Copy link
Contributor Author

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

jdonszelmann
jdonszelmann commented Aug 28, 2025
View reviewed changes
tests/ui/recursion_limit/invalid_digit.stderr
@@ -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

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?

@fmease
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.

@fmease fmease mentioned this pull request Aug 28, 2025
Merged
@bors
Copy link
Collaborator

bors commented Aug 28, 2025

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

@jdonszelmann
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

@rustbot
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.

@jdonszelmann
Copy link
Contributor Author

@rustbot ready

fmease
fmease approved these changes Aug 29, 2025
View reviewed changes
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.

r=me after nits

View changes since this review

compiler/rustc_attr_parsing/src/attributes/crate_level.rs
const ON_DUPLICATE: OnDuplicate<S> = OnDuplicate::WarnButFutureError;
const TEMPLATE: AttributeTemplate = template!(NameValueStr: "N");

// FIXME: recursion limit is allowed on all targets and ignored,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// FIXME: recursion limit is allowed on all targets and ignored,
// FIXME: move size limit is allowed on all targets and ignored,

compiler/rustc_attr_parsing/src/attributes/crate_level.rs
impl<S: Stage> SingleAttributeParser<S> for MoveSizeLimitParser {
const PATH: &[Symbol] = &[sym::move_size_limit];
const ATTRIBUTE_ORDER: AttributeOrder = AttributeOrder::KeepOutermost;
const ON_DUPLICATE: OnDuplicate<S> = OnDuplicate::WarnButFutureError;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if you want to do that in this PR but #![move_size_limit] is still unstable under feature large_assignments (RUST-83518), so theoretically we could just bump this to OnDuplicate::Error.

compiler/rustc_attr_parsing/src/attributes/crate_level.rs

// FIXME: recursion limit is allowed on all targets and ignored,
// even though it should only be valid on crates of course
const ALLOWED_TARGETS: AllowedTargets = AllowedTargets::AllowList(ALL_TARGETS);
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, this could be bumped to AllowedTargets::AllowList(&[Target::Crate]) (is that how it's done?) because it's still unstable.

compiler/rustc_attr_parsing/src/attributes/crate_level.rs
impl<S: Stage> SingleAttributeParser<S> for PatternComplexityLimitParser {
const PATH: &[Symbol] = &[sym::pattern_complexity_limit];
const ATTRIBUTE_ORDER: AttributeOrder = AttributeOrder::KeepOutermost;
const ON_DUPLICATE: OnDuplicate<S> = OnDuplicate::WarnButFutureError;
Copy link
Member

Choose a reason for hiding this comment

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

Since #![pattern_complexity_limit] is gated behind the internal feature rustc_attrs (for some reason, lol), this could be upgraded to OnDuplicate::Error.

compiler/rustc_attr_parsing/src/attributes/crate_level.rs

// FIXME: recursion limit is allowed on all targets and ignored,
// even though it should only be valid on crates of course
const ALLOWED_TARGETS: AllowedTargets = AllowedTargets::AllowList(ALL_TARGETS);
Copy link
Member

Choose a reason for hiding this comment

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

… and this one to AllowList(&[Level::Crate]) (if that's the correct policy) for the same reason.

compiler/rustc_attr_parsing/src/context.rs
@@ -677,7 +680,7 @@ pub enum ShouldEmit {
///
/// Only relevant when early parsing, in late parsing equivalent to `ErrorsAndLints`.
/// Late parsing is never fatal, and instead tries to emit as many diagnostics as possible.
EarlyFatal,
EarlyFatal { also_emit_lints: bool },
/// The operation will emit errors and lints.
/// This is usually what you need.
ErrorsAndLints,
Copy link
Member

Choose a reason for hiding this comment

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

(preexisting) docs of variant Nothing mention nonexistent variant Emit.

compiler/rustc_interface/src/passes.rs
@@ -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)

compiler/rustc_interface/src/passes.rs
DUMMY_SP,
rustc_ast::node_id::CRATE_NODE_ID,
None,
ShouldEmit::EarlyFatal { also_emit_lints: false },
Copy link
Member

Choose a reason for hiding this comment

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

Why false? Since it's non-obvious to me, could you add a small comment please, thanks!

tests/ui/feature-gates/issue-43106-gating-of-builtin-attrs.stderr
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.

tests/ui/recursion_limit/invalid_digit.stderr
@@ -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

Assignees

@fmease fmease

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.
6 participants
@jdonszelmann @rustbot @rust-log-analyzer @scrabsha @bors @fmease