Skip to content
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

[WIP] Implement RFC 1560. #32213

Closed
wants to merge 8 commits into from
Closed

[WIP] Implement RFC 1560. #32213

wants to merge 8 commits into from

Conversation

jseyfried
Copy link
Contributor

@jseyfried jseyfried commented Mar 12, 2016 •
edited
Loading

This prototypes rust-lang/rfcs#1560 (cc #35120).

More specifically,

  • Items and named imports shadow glob imports.
  • Multiple globs can import the same name if the name is unused or the imports are shadowed.
  • Multiple globs can import the same name if the imports are of the same item (following re-exports).
    • The visibility of such a name is the maximum visibility of the imports.
    • Equivalently, adding a glob import will never reduce the visibility of a name, nor will removing one increase it.
  • Non-prelude private imports can be used wherever we currently allow private items to be used.
    • Prelude-imported names are unaffected, i.e. they continue to be usable only in lexical scopes.
  • Globs import all visible names, not just public names.
    • Equivalently, glob importing from an ancestor module imports all of the ancestor's names, and glob importing from other modules is unchanged.
    • The visibility of a glob-imported name is the minimum visibility of the glob itself and the name in the imported module. In particular, pub use super::* is equivalent to useing the private names and pub useing the public names from super (modulo shadowing). See this comment from resolve: Privacy rules for re-exports can be too restrictive #31783.

This implementation still needs documentation, tests, and more precise diagnostics (most importantly, an error for when an ambiguous name is used), but other than that it is complete.

@jseyfried jseyfried force-pushed the new_import_semantics branch 4 times, most recently from e333048 to c759b3e Compare March 12, 2016 13:26
@retep998
Copy link
Member

Also if you could test it on Windows with winapi since that makes use of recursive globs.

@jseyfried jseyfried force-pushed the new_import_semantics branch 2 times, most recently from 4ebba6d to 5c3ab23 Compare March 12, 2016 22:15
@bors
Copy link
Contributor

bors commented Mar 13, 2016

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

@jseyfried
Copy link
Contributor Author

cc @petrochenkov

@alexcrichton
Copy link
Member

Unfortunately I broke make dist in #32133, so crater isn't working as it can't build a compiler from this commit. Can you rebase on #32217 once it lands? That should enable a crater run!

@bors
Copy link
Contributor

bors commented Mar 13, 2016

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

@nrc
Copy link
Member

nrc commented Mar 14, 2016

Just a note that the proposed changes are a draft RFC that hasn't been PR'ed yet, we should wait for the RFC to be approved (and of course to be a PR first) before landing this.

I would be curious to see a crater run in the meantime though.

@jseyfried
Copy link
Contributor Author

we should wait for the RFC to be approved before landing this

I know; this is a ways off from being ready to land regardless.

@jseyfried
Copy link
Contributor Author

@alexcrichton I had to rebase anyway -- this should be ready for a crater run now!

@alexcrichton
Copy link
Member

r? @nikomatsakis (or @nrc, ... or others!)

@alexcrichton
Copy link
Member

(also started a crater run)

@alexcrichton alexcrichton added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Mar 14, 2016
@jseyfried
Copy link
Contributor Author

(also started a crater run)

Thanks!

@alexcrichton
Copy link
Member

Oh dear, looks like quite a few regressions :(

@jonas-schievink
Copy link
Contributor

Most of them seem to be caused by the use of bitflags

@jseyfried
Copy link
Contributor Author

All of the root regressions appear to be due to a bug that I fixed in the above commit.

I had generalized the last bullet point ("The visibility of a glob-imported name is the minimum visibility of the glob itself and the name in the imported module") to apply to single imports as well. This won't make a difference once the warning cycle from #31362 is complete, but until then a pub single import of a private extern crate should be public, not private.

@alexcrichton
Copy link
Member

Scheduled another crater run

@alexcrichton
Copy link
Member

Hurray, zero regressions!

@alexcrichton alexcrichton removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Mar 15, 2016
@jseyfried
Copy link
Contributor Author

Excellent. It also looks like these changes fix two crates that were broken by new names in upstream crates.

@bors
Copy link
Contributor

bors commented Mar 17, 2016

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

@bors
Copy link
Contributor

bors commented Apr 6, 2016

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

@jseyfried jseyfried mentioned this pull request Apr 11, 2016
Merged
@bors
Copy link
Contributor

bors commented Apr 13, 2016

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

@jseyfried jseyfried force-pushed the new_import_semantics branch 3 times, most recently from 4c2626e to 0d86604 Compare April 14, 2016 06:54
@bors
Copy link
Contributor

bors commented Apr 17, 2016

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

@alexcrichton
Copy link
Member

@jseyfried this has been inactive for awhile now, should we close or try to rebase + land?

@jseyfried
Copy link
Contributor Author

jseyfried commented Jul 19, 2016 •
edited
Loading

@alexcrichton I'm planning on rebasing, finishing this up, and maybe feature gating once RFC 1560 goes into FCP, which should be any day now.

@alexcrichton
Copy link
Member

Ok, awesome!

@jseyfried jseyfried changed the title [WIP] Changes to name resolution rules (shadowable globs, item-like private imports, etc.) [WIP] Implement RFC 1560. Jul 29, 2016
@jseyfried jseyfried mentioned this pull request Jul 29, 2016
Merged
bors added a commit that referenced this pull request Aug 5, 2016
@bors
Auto merge of #35116 - jseyfried:groundwork_for_new_import_semantics,…
7bf54f9 
… r=nrc

resolve: diagnostics improvement and groundwork for RFC 1560

Fixes #35115, fixes #35135, and lays groundwork for #32213 (cc #35120).
r? @nrc
@nikomatsakis
Copy link
Contributor

@jseyfried going to close this PR due to inactivity. Feel free to re-open though. :)

@jseyfried jseyfried mentioned this pull request Aug 18, 2016
Merged
@jseyfried
Copy link
Contributor Author

I finished this in #35894.

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

@nikomatsakis nikomatsakis

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jseyfried @retep998 @bors @alexcrichton @nrc @jonas-schievink @nikomatsakis