-
Notifications
You must be signed in to change notification settings - Fork 13k
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
parser: simplify directory ownership semantics #37602
Conversation
cc @rust-lang/lang @matklad |
543ceb5
to
9828456
Compare
Started a crater run. |
Results: https://gist.github.com/nikomatsakis/ad150e91da19d7a86d8501a11f2b0a0c
Root regressions to be investigated:
|
@jseyfried so at least a few of those are real regr. We have to decide now: to warning period or what. cc @rust-lang/lang -- this PR makes some changes to rationalize various aspects of when a module "has a directory" and not (i.e., when you can do |
@nikomatsakis the openssl errors are rust-lang/cargo#3268 |
@nikomatsakis This looks like a good case for applying the general guidelines. In general I'm 👍 going forward with these changes. How hard is a warning period to do in this case? If it's relatively easy, we should do it. |
I'd encourage you to make any breakage here a transitionary warning. We must continue to do our utmost to reduce breakage pain for users. Small breakage adds up. |
It looks like all the breakage is due to |
Definitely we should do a warning period. |
19030ba
to
0b060ac
Compare
I started a warning cycle in the above commit. Since it's not easy to lint from the parser, it is just an ordinary warning with a note about future compatibility. |
if paths.path_exists { | ||
if legacy_warn { | ||
err.note( | ||
"this was erroneously allowed and will become a hard error in a future release" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I think we could do an actual lint here -- can't we leave a little note on the AST or something and then issue the lint later, in the lint pass?
At minimum, though, can you open a "forward compat" issue with the appropriate labels and structure and mention it in this message? Roughly follow the protocol described in RFC 1589
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll make it a real lint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
ping @jseyfried -- still planning to make it a real lint? |
0b060ac
to
75eecc5
Compare
@nikomatsakis just fininshed (got sidetracked by some higher-priority stuff) |
The lint is temporary, but still could you rename it to follow lint naming conventions? |
75eecc5
to
533068f
Compare
@petrochenkov yeah, renamed to |
☔ The latest upstream changes (presumably #37824) made this pull request unmergeable. Please resolve the merge conflicts. |
@jseyfried r=me after rebase |
533068f
to
fa8c53b
Compare
@bors r=nikomatsakis |
📌 Commit fa8c53b has been approved by |
parser: simplify directory ownership semantics This PR simplifies the semantics of "directory ownership". After this PR, - a non-inline module without a `#[path]` attribute (e.g. `mod foo;`) is allowed iff its parent module/block (whichever is nearer) is a directory owner, - an non-inline module is a directory owner iff its corresponding file is named `mod.rs` (c.f. [comment](#32401 (comment))), - a block is never a directory owner (c.f. #31534), and - an inline module is a directory owner iff either - its parent module/block is a directory owner (again, c.f. #31534), or - it has a `#[path]` attribute (c.f. #36789). These semantics differ from today's in three orthogonal ways: - `#[path = "foo.rs"] mod foo;` is no longer a directory owner. This is a [breaking-change]. - #36789 is generalized to apply to modules that are not directory owners in addition to blocks. - A macro-expanded non-inline module is only allowed where an ordinary non-inline module would be allowed. Today, we incorrectly allow macro-expanded non-inline modules in modules that are not directory owners (but not in blocks). This is a [breaking-change]. Fixes #32401. r? @nikomatsakis
Sorry, I don't understand what I need to do to change in ring here. I don't want to name all the modules' files mod.rs because it's ridiculous to have dozens of files with the same name in one project. Of course some of them have submodules. What's the alternative? |
@briansmith You can declare the module hierarchy recursively, even the entire crate's modules in |
This PR simplifies the semantics of "directory ownership". After this PR,
#[path]
attribute (e.g.mod foo;
) is allowed iff its parent module/block (whichever is nearer) is a directory owner,mod.rs
(c.f. comment),#[path]
attribute (c.f. Allow more non-inline modules in blocks #36789).These semantics differ from today's in three orthogonal ways:
#[path = "foo.rs"] mod foo;
is no longer a directory owner. This is a [breaking-change].Fixes #32401.
r? @nikomatsakis