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

libsyntax: Restrict where non-inline modules can appear (fixes #29765) #31534

Merged
merged 2 commits into from Feb 16, 2016

Conversation

Projects
None yet
4 participants
@jseyfried
Contributor

jseyfried commented Feb 10, 2016

This PR disallows non-inline modules without path annotations that are either in a block or in an inline module whose containing file is not a directory owner (fixes #29765).
This is a [breaking-change].
r? @nikomatsakis

@jseyfried

This comment has been minimized.

Contributor

jseyfried commented Feb 11, 2016

Disallowing non-inline modules in blocks broke librustc_back -- there was a macro defined and invoked inside a function that expanded to code that declared non-inline modules. I fixed it by moving the macro and its invocation out of the function.

This doesn't look like it is a common pattern, but it may be a good idea to do a crater run to be sure. We could continue allowing non-inline modules in blocks (unless the containing file is not a directory owner, which would still be enforced by my first commit) or make it a warning/lint. I don't have a strong opinion on this issue, but it looks like the consensus in #29765 was to disallow non-inline modules in blocks.

cc @aturon @matklad

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Feb 11, 2016

Should we do a crater run do you think?

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Feb 11, 2016

Or -- alternatively -- just start out with added a lint.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Feb 11, 2016

(Seems like it's prob not needed here though.)

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Feb 11, 2016

OK well I kicked off a crater run just for kicks. This patch looks good though. r=me once the crater run is done.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Feb 11, 2016

cc @brson -- this is a bug fix but a breaking change, I'm not sure what tags we were supposed to use but I added some :)

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Feb 12, 2016

I got one regression from a crater run: https://gist.github.com/nikomatsakis/f30f5a095b4576411201

(cargo)

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Feb 12, 2016

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Feb 12, 2016

📌 Commit 69f3a1b has been approved by nikomatsakis

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 12, 2016

:(

@jseyfried

This comment has been minimized.

Contributor

jseyfried commented Feb 12, 2016

I wrote a PR for cargo to fix the regression.

@bors

This comment has been minimized.

Contributor

bors commented Feb 13, 2016

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

@jseyfried

This comment has been minimized.

Contributor

jseyfried commented Feb 13, 2016

@nikomatsakis rebased

@jseyfried

This comment has been minimized.

Contributor

jseyfried commented Feb 16, 2016

@nikomatsakis Do you still want to merge this?

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Feb 16, 2016

@jseyfried sorry, didn't see the problem, yes I would like to merge

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Feb 16, 2016

@alexcrichton

:(

I can't tell how to interpret this smiley...

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 16, 2016

Ah yes, I can clarify! I saw that only Cargo was mentioned, and that felt like Cargo got some sort of prize or something, so I felt compelled to comment. After seeing the prize it won, however, I deleted the half-written essay of acceptance and felt compelled to continue writing something, so I settled for an emoticon, and ":(" seemed appropriate at the time.

In other words, I wouldn't try to interpret :)

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Feb 16, 2016

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Feb 16, 2016

📌 Commit d21e908 has been approved by nikomatsakis

bors added a commit that referenced this pull request Feb 16, 2016

Auto merge of #31534 - jseyfried:restrict_noninline_mod, r=nikomatsakis
This PR disallows non-inline modules without path annotations that are either in a block or in an inline module whose containing file is not a directory owner (fixes #29765).
This is a [breaking-change].
r? @nikomatsakis
@bors

This comment has been minimized.

Contributor

bors commented Feb 16, 2016

⌛️ Testing commit d21e908 with merge 45652d9...

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 16, 2016

@bors: retry force

@bors

This comment has been minimized.

Contributor

bors commented Feb 16, 2016

⌛️ Testing commit d21e908 with merge 9658645...

bors added a commit that referenced this pull request Feb 16, 2016

Auto merge of #31534 - jseyfried:restrict_noninline_mod, r=nikomatsakis
This PR disallows non-inline modules without path annotations that are either in a block or in an inline module whose containing file is not a directory owner (fixes #29765).
This is a [breaking-change].
r? @nikomatsakis

@bors bors merged commit d21e908 into rust-lang:master Feb 16, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@llogiq llogiq referenced this pull request Feb 22, 2016

Closed

TWIR 119's CotW + PRs #172

@jseyfried jseyfried deleted the jseyfried:restrict_noninline_mod branch Nov 4, 2016

bors added a commit that referenced this pull request Nov 22, 2016

Auto merge of #37602 - jseyfried:directory_ownership, r=nikomatsakis
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

@alygin alygin referenced this pull request Jan 31, 2017

Merged

Mod completion #975

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment