Fix nonsensical #[path] attributes being silently allowed

[krishkumarwork3-beep]

View all comments

r? @jdonszelmann

Fixes #157260

Previously, the compiler silently accepted nonsensical uses of the #[path]
attribute. This PR makes these into hard errors.

What are the positions where nonsensical #[path] attributes were being
silently allowed, but are now loudly rejected?

Position 1: #[path] on an inline module with no external submodules

#[path = "foo.rs"]
mod inline_module {}
// Previously accepted, now:
// error: attribute `#[path]` is useless on inline modules

Position 2: #[path] on an inline module whose submodules are also inline

#[path = "foo.rs"]
mod inline_with_inline_sub {
    mod inner {} // inline, not external
}
// Previously accepted, now:
// error: attribute `#[path]` is useless on inline modules

Position 3: #![path] inside a module with no external submodules

mod foo {
    #![path = "some_dir"]
    // no external submodules
}
// Previously accepted, now:
// error: attribute `#[path]` is useless here as there are no nested external modules

Position 4: #![path] inside a module where the submodule is inline (has body {})

mod thread_inline_sub {
    #![path = "thread_files"]
    #[path = "tls.rs"]
    mod local_data {} // inline, has body
}
// Previously accepted, now:
// error: attribute `#[path]` is useless here as there are no nested external modules
// error: attribute `#[path]` is useless on inline modules

What remains valid (no error):

// Valid: external module
#[path = "foo.rs"]
mod foo;

// Valid: inline module WITH external submodules (semicolon = external)
mod thread {
    #![path = "thread_files"]
    #[path = "tls.rs"]
    mod local_data; // external (semicolon), path is meaningful
}

Is this a breaking change?
Yes — code that previously compiled without errors will now emit hard errors.

[rustbot]

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @theemathas (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[Kivooeo]

Hi @krishkumarwork3-beep, thanks for your PR. I’m afraid @theemathas probably won’t be able to review it, as we need someone from the compiler team for this, so I’ll re-assign another member.

@rustbot reroll

[krishkumarwork3-beep]

@rustbot review

The PR - tidy check appears to be failing due to a tidy tool crash
(process exit code 1 with a panic stack trace) rather than a formatting
issue. I ran x.py fmt locally and it completed successfully.
Could someone re-trigger the CI?

[Kivooeo]

The CI is failing due to the absence of a newline in your test file.

[JonathanBrouwer]

(You can reproduce this locally with ./x test tidy)

[krishkumarwork3-beep]

thanks @JonathanBrouwer and @Kivooeo
i have fixed the newline/line-ending issue, ran ./x test tidy locally, and force-pushed the updated changes.

[jdonszelmann]

This is also a breaking change. Do we want to/need to crater?

[jdonszelmann]

Shouldn't we be able to just look whether a path attr is present on the nested module? That way we don't have to go through the source map

View changes since the review

[jdonszelmann]

With find_attr!()

[krishkumarwork3-beep]

Good point! However, checking for a #[path] attribute on the nested module doesn't tell us if the module is external , a module can be external (mod foo;) without having a #[path] attribute, and an inline module (mod foo {}) can have one. The actual distinction between inline and external is whether the module body is in the same file, which is what the source map check captures. I've factored the source map comparison into a shared is_inline closure to avoid the duplication you pointed out. Happy to revisit if you have a different approach in mind!

[jdonszelmann]

Same here. In fact, even if looking at path attrs doesn't work, this check in the source map seems very similar to the one below. Might want to factor it out into a functionm

View changes since the review

[krishkumarwork3-beep]

Done, I have factored the source map file name comparison into a local is_inline closure reused for both the parent and nested module checks in latest commit

[jdonszelmann]

I believe we have a .hir_item() method that returns an option?

View changes since the review

[krishkumarwork3-beep]

I looked into this - tcx.hir_item() takes an ItemId rather than a HirId, so it can't be used directly here. I've replaced the hir_node + match pattern with let hir::Node::Item(item) = self.tcx.hir_node(hir_id) else { return; } which achieves the same clean early-return pattern. Let me know if you had a different method in mind!
Thanks @jdonszelmann for your suggestions

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[Kivooeo]

Do we want to/need to crater?

If this is the change we want, the answer is yes

[jieyouxu]

And it needs to be a FCP by lang if it's a breaking change (depending on fallout, whether a FCW lint w/ migration period is needed).

[jieyouxu]

Suggestion: please edit your PR description to more meaningfully reflect what is being changed about the language (reviewers do not need a list of files being changed; they can see that through the diff already). Please describe e.g.:

• Is this a breaking change?
• Can you give an example of what previously worked, but now won't after this PR?
• What are the positions where nonsensical #[path] attributes were being silently allowed, but are now loudly rejected?

For an example, seee #145463 (comment)

View changes since the review

[krishkumarwork3-beep]

yes @jieyouxu i have updated the PR description as suggested by you
thanks for the suggestion ,I will take care of it in future

[Kivooeo]

And it needs to be a FCP by lang

In that case I think it can be lang nominated right?

[jieyouxu]

In that case I think it can be lang nominated right?

Yes, but please only nominate for lang after there's a more concrete proposal (incl. breakage estimation via crater analysis) and something that's more explicit (e.g. see linked PR summary) so lang doesn't have to dig through context.

[theemathas]

You claim at #157260 (comment) that this code will not produce a warning. This test shows that the code produces a hard error instead. Could you explain?

View changes since the review

[krishkumarwork3-beep]

mod thread_inline_sub { //~ ERROR attribute #[path] is useless here as there are no nested external modules
#![path = "thread_files"]
#[path = "tls.rs"]
mod local_data {} //~ ERROR attribute #[path] is useless on inline modules
} is different from
mod thread_inline_sub {
#![path = "thread_files"]
#[path = "tls.rs"]
mod local_data;
} what @JonathanBrouwer asked for
in the case 1
mod local_data {} has a body {} → it's inline → compiler already knows its content, #[path] is meaningless ,since local_data is inline (not external), thread_inline_sub has no external submodules → #![path] is also meaningless so a hard error is produced
and
in case 2
mod local_data; has a semicolon → it's external → compiler needs #[path = "tls.rs"] to find the file,since local_data is external, thread_inline_sub has an external submodule → #![path = "thread_files"] is meaningful as a directory hint so no error should come

[rustbot]

Requested reviewer is already assigned to this pull request.

Please choose another assignee.

[bjorn3]

I did say this should at most be a warning. The following is reasonable to me:

#[path = "thread_files"]
mod thread {
    #[cfg(feature = "enable_tls")]`
    #[path = "tls.rs"]
    mod local_data; // external (semicolon), path is meaningful
}

it would be weird if that suddenly emits a hard error when enable_tls is not enabled.

[krishkumarwork3-beep]

I did say this should at most be a warning. The following is reasonable to me:

#[path = "thread_files"]
mod thread {
    #[cfg(feature = "enable_tls")]`
    #[path = "tls.rs"]
    mod local_data; // external (semicolon), path is meaningful
}

it would be weird if that suddenly emits a hard error when enable_tls is not enabled.

@bjorn3 you're right - cfg-gated external submodules make this more subtle.
Since cfg-disabled items are removed before this check runs, the current implementation can incorrectly treat some cfg-gated external submodules as absent, which could lead to surprising hard errors in configurations where the external module is disabled.
Given that, a warning/lint-based approach may make more sense here, at least initially, especially considering the breaking-change implications.

[workingjubilee]

@krishkumarwork3-beep Hello. What tools did you arrive to identify this changeset and produce the diff?

[rustbot]

This PR was rebased onto a different main 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.

[krishkumarwork3-beep]

@krishkumarwork3-beep Hello. What tools did you arrive to identify this changeset and produce the diff

Hello @workingjubilee
I found this issue by browsing the rust-lang/rust issue tracker for E-easy tagged issues. The issue (#157260) was
already identified and described by the issue author.

To understand the codebase and approach, I used ChatGPT/Claude to help me navigate the compiler internals. For the implementation, I had prior knowledge of a similar validation function check_doc_search_unbox in check_attr.rs which validates #[doc(search_unbox)] attributes using the HIR visitor pattern. I used that as a reference to create a similar check_path_attribute function that checks whether #[path] and #![path] attributes are being used in meaningful positions by inspecting the HIR to determine if a module is inline and whether it has external submodules.

I used VS Code for editing and x.py build / x.py test to buildand test the changes locally on Windows.

[krishkumarwork3-beep]

@rustbot I have resolved the merge conflicts

[krishkumarwork3-beep]

I did say this should at most be a warning. The following is reasonable to me:

#[path = "thread_files"]
mod thread {
    #[cfg(feature = "enable_tls")]`
    #[path = "tls.rs"]
    mod local_data; // external (semicolon), path is meaningful
}

it would be weird if that suddenly emits a hard error when enable_tls is not enabled.

I've now understood exactly why this happens.

Our check runs on HIR, but #[cfg] evaluation and stripping happens before HIR is built. So in this case:

#[path = "thread_files"]
mod thread {
    #[cfg(feature = "enable_tls")]  // feature not enabled
    #[path = "tls.rs"]
    mod local_data;  // stripped from HIR entirely
}

Since enable_tls is not enabled, mod local_data; is completely removed before HIR is constructed. When our check runs,
module.item_ids is empty, has_nested_external_modules is false, and the error fires incorrectly.

Without #[cfg], mod local_data; exists in HIR, has_nested_external_modules is true, and no error is emitted —
which is correct. So below case doesn't give error

#[path = "thread_files"]
mod thread {
    #[path = "tls.rs"]
    mod local_data;  // this EXISTS in HIR
}

To fix this properly, the check would need to move to an earlier compiler stage (AST passes) where cfg has not yet been evaluated and all items are still visible. Would that be the right direction, or would you suggest changing this to a warning instead of a hard error to avoid this issue?

[workingjubilee]

A lot of the reason we leave E-easy issues like this one open is so that new contributors can try their hand at them and learn from the process. Using an LLM to drive so much of the PR denies both yourself and others the opportunity to learn. An LLM, despite using "machine learning", does not truly learn, as it is a stateless function on a context window.

And by learn, I mean you are asking a question answered by the statement you are responding to, and claiming you had "prior knowledge" of a codebase you admit you are new to. If we were mentoring someone so much to get them over the line, think it would be better if we were investing such in the original contributor who had claimed the issue.

[krishkumarwork3-beep]

A lot of the reason we leave E-easy issues like this one open is so that new contributors can try their hand at them and learn from the process. Using an LLM to drive so much of the PR denies both yourself and others the opportunity to learn. An LLM, despite using "machine learning", does not truly learn, as it is a stateless function on a context window.

And by learn, I mean you are asking a question answered by the statement you are responding to, and claiming you had "prior knowledge" of a codebase you admit you are new to. If we were mentoring someone so much to get them over the line, think it would be better if we were investing such in the original contributor who had claimed the issue.

@workingjubilee Thanks for the feedback. I understand the concern and appreciate you taking the time to explain it.

I've been learning Rust for a while, but I'm still new to many parts of the compiler codebase. I used an LLM primarily to help navigate the repository and identify potentially relevant areas to investigate, since I wasn't yet familiar with where all of the attribute checking logic lived.

After @JonathanBrouwer suggested looking in check_attr.rs, I spent time reading through that file and trying to relate the issue to existing checks. For example, I searched for and followed references related to path, #[path], inline module, emit_err, similar attribute validation code, and other check_* functions to understand how comparable diagnostics were implemented. I was using those searches to explore the codebase and understand the existing patterns rather than having the solution generated for me.

I completely understand the concern about preserving the learning value of E-easy issues. My intention was not to misrepresent my understanding or bypass that learning process.

If anything, this issue ended up teaching me quite a bit about the compiler's attribute checking infrastructure and diagnostics. While working on it, I spent time understanding how diagnostics are defined and emitted, how the structures in errors.rs are used to construct error messages, and how the checks in check_attr.rs fit into the broader attribute validation flow.

I also ended up reading and learning from existing UI tests, including files such as issue-43106-gating-of-builtin-attrs.rs, to understand how similar diagnostics are tested and how expected output is maintained. Through the review process, I learned more about updating UI tests, interpreting test failures, and keeping diagnostics consistent with existing compiler behavior.

I learned quite a bit about the Rust compiler contribution workflow: running and debugging x.py test tidy, understanding tidy failures, working with CI feedback, updating tests after diagnostic changes, and iterating on a patch based on reviewer comments.

Thank you for the explanation and for reviewing the PR.

If you have any suggestions on how I can better approach or communicate my investigation and understanding in future compiler contributions, I'd appreciate the feedback. I'm still learning the codebase and would like to improve my contribution process.