Skip to content

Improve diagnostics: update note and add help message #147395

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 1 commit into
base: master
Choose a base branch
from
Open

Improve diagnostics: update note and add help message #147395

wants to merge 1 commit into from
+24 −19

Conversation

reddevilmidzy
Copy link
Contributor

@reddevilmidzy reddevilmidzy commented Oct 6, 2025 •
edited by rustbot
Loading

I moved the content from the note to a help message, as it seemed more appropriate there, and then added new information to the note(Modules are usually placed outside of blocks, at the top level of the file)!

resolve: #147314

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 6, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 6, 2025

r? @jackh726

rustbot has assigned @jackh726.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Kivooeo
Kivooeo reviewed Oct 6, 2025
View reviewed changes
Copy link
Member

@Kivooeo Kivooeo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, some nits as was discussed before

View changes since this review

compiler/rustc_expand/messages.ftl Outdated
.note = if there is a `mod {$name}` elsewhere in the crate already, import it with `use crate::...` instead
expand_module_in_block =
cannot declare a non-inline module inside a block unless it has a path attribute
Copy link
Member

Choose a reason for hiding this comment

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

As was little discussed here #147314 (comment), in my opinion it worth to use other than "non-inline" word here, this one is not obvious for non native English speakers, so we could use other one that will be more simple

cc @hkBst

Copy link
Contributor Author

@reddevilmidzy reddevilmidzy Oct 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

thank you for the review!

I’ve replaced “non-inline module” with “external module” and updated the existing test file name and comments accordingly. I also noticed that “non-inline” is used here

Would it be a good idea to change this message too?

(I’ll squash the commits after the review is finished)

Copy link
Member

@Kivooeo Kivooeo Oct 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

"non-inline modules" is used for a lot of places, I'm not sure if we want to replace them too for now, but at least this one message is need beacuse I think a lot of newcomers can do this mistake and get this error, I believe that start with this place will be good, and then in future we will see if this is necessary to change in other places too

compiler/rustc_expand/messages.ftl Outdated
cannot declare a non-inline module inside a block unless it has a path attribute
.note = maybe `use` the module `{$name}` instead of redeclaring it
.help = maybe `use` the module `{$name}` instead of redeclaring it
.note = modules are usually placed outside of blocks, at the top level of the file
Copy link
Member

@Kivooeo Kivooeo Oct 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Also this note as far as I can say shouldn't say just "modules" but "'verb that describe that this is not an inline module' modules"

Copy link
Member

Choose a reason for hiding this comment

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

For some reasons github removed part before second modules because I put it in angle brackets, I updated comment

@Kivooeo
Copy link
Member

Kivooeo commented Oct 6, 2025

This actually looks good to me, but unfortunately can't rollup since have no rights to do this right now, have to wait when someone else approve it

@reddevilmidzy
Copy link
Contributor Author

Thanks for reviewing! I’ll wait for approval then :)

@jackh726
Copy link
Member

jackh726 commented Oct 6, 2025

@bors r=Kivooeo,jackh726 rollup

@bors
Copy link
Collaborator

bors commented Oct 6, 2025

📌 Commit 002348e has been approved by Kivooeo,jackh726

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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 6, 2025
@fmease
Copy link
Member

fmease commented Oct 6, 2025

https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/Rename.20.22non-inline.20modules.22.20to.20.22external.20modules.22
@bors r-

@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 Oct 6, 2025
@Kivooeo
Copy link
Member

Kivooeo commented Oct 6, 2025 •
edited
Loading

I created a topic (#general > Rename "non-inline modules" to "external modules") and we decided to stick with "file modules" rather than "external modules" because of confusion with extern keyword

Would it be a good idea to change this message too?

And yes, you can open follow up PR or add the changes to this one, both are fine

@reddevilmidzy
Copy link
Contributor Author

thanks I'll open follow up PR
and I linked the wrong line earlier — it should’ve been this one

@rustbot ready

@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 Oct 6, 2025
@jackh726
Copy link
Member

jackh726 commented Oct 6, 2025

@bors delegate=Kivooeo

@bors
Copy link
Collaborator

bors commented Oct 6, 2025

✌️ @Kivooeo, you can now approve this pull request!

If @jackh726 told you to "r=me" after making some further change, please make that change, then do @bors r=@jackh726

@Kivooeo
Copy link
Member

Kivooeo commented Oct 6, 2025

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Oct 6, 2025

📌 Commit 02126ad has been approved by Kivooeo

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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 6, 2025
@fmease
Copy link
Member

fmease commented Oct 6, 2025

@bors r-
@bors r=Kivooeo,jackh726

@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 Oct 6, 2025
@bors
Copy link
Collaborator

bors commented Oct 6, 2025

📌 Commit 02126ad has been approved by Kivooeo,jackh726

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 Oct 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@Kivooeo Kivooeo Kivooeo left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@jackh726 jackh726

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

Diagnostic for non-inline modules inside blocks should encourage moving it out of the block
6 participants
@reddevilmidzy @rustbot @Kivooeo @jackh726 @bors @fmease