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

Stabilize `uniform_paths` #56759

Merged
merged 6 commits into from Jan 12, 2019

Conversation

@petrochenkov
Copy link
Contributor

petrochenkov commented Dec 12, 2018

Address all the things described in #56417.

Assign visibilities to macro_rules items - pub to macro_export-ed macros and pub(crate) to non-exported macros, these visibilities determine how far these macros can be reexported with use.

Prohibit use of reexported inert attributes and "tool modules", after renaming (e.g. use inline as imported_inline) their respective tools and even compiler passes won't be able to recognize and properly check them.

Also turn use of uniform paths in 2015 macros into an error, I'd prefer to neither remove nor stabilize them right away because I still want to make some experiments in this area (uniform path fallback was added to 2015 macros used on 2018 edition in #56053 (comment)).

UPDATE: The last commit also stabilizes the feature uniform_paths.

Closes #53130
Closes #55618
Closes #56326
Closes #56398
Closes #56417
Closes #56821
Closes #57252
Closes #57422

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Dec 12, 2018

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 18, 2018

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

@petrochenkov petrochenkov force-pushed the petrochenkov:prestabuni branch from 39daf51 to 42028d2 Dec 27, 2018

@petrochenkov petrochenkov changed the title resolve: Prepare uniform paths to stabilization Stabilize `uniform_paths` Dec 27, 2018

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Dec 27, 2018

@Centril
I've added one more commit actually stabilizing the feature.
Could you start an FCP?

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Dec 27, 2018

Stabilization report

Feature name: #![feature(uniform_paths)]
Version target: 1.32 (2019-01-18)

Implemented in #52923 (v1) and #55884 (v2) and landed 4.5 and 1.5 months ago respectively.
Tracking issues are #53130, #55618 and #56417.
Documentation issue is rust-lang-nursery/reference#487.
Tests for the feature can be found in src/test/run-pass/uniform-paths and src/test/ui/rust-2018/uniform-paths.

What is being stabilized

If import path resolves without errors, then it's resolved in exactly same way as a non-import path.

Possible extra errors (e.g. ambiguities) in import paths compared to other paths are caused by the fact that import paths are resolved early, when module structure of the crate is still in flux.
Some of the errors are artificial and can be relaxed backward-compatibly in the future, making import paths even closer to non-import paths.

Right now an ambiguity error is reported if the number of different candidates in scope for the first import segment (foo in use foo::bar::baz;) is more than one.
Multiple candidates with same definition don't produce an error:

struct S; // Candidate 1
fn main() {
    use self::S; // Candidate 2
    {
        use self::S; // Candidate 3
        use S as _; // Not an error
    }
}

Known issues

Known deviations of macro paths and 2018 import paths from the "uniform path" model

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Dec 27, 2018

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0a84eb9d:start=1545872962920320386,finish=1545872965109241268,duration=2188920882
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
tidy check
[00:03:07] * 568 error codes
[00:03:07] * highest error code: E0721
[00:03:07] * 244 features
[00:03:08] Stray file with UI testing output: "/checkout/src/test/ui/rust-2018/uniform-paths/issue-54390.stderr"
[00:03:08] some tidy checks failed
[00:03:08] 
[00:03:08] 
[00:03:08] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:03:08] 
[00:03:08] 
[00:03:08] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:08] Build completed unsuccessfully in 0:00:44
[00:03:08] Build completed unsuccessfully in 0:00:44
[00:03:08] Makefile:69: recipe for target 'tidy' failed
[00:03:08] make: *** [tidy] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:11dcbaa8
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Thu Dec 27 01:12:42 UTC 2018
---
travis_time:end:09ff3ff3:start=1545873163097760936,finish=1545873163102740732,duration=4979796
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:12715314
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0285b86a
travis_time:start:0285b86a
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0aa348f0
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@petrochenkov petrochenkov force-pushed the petrochenkov:prestabuni branch from 42028d2 to 3a2e4db Dec 27, 2018

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 27, 2018

@petrochenkov

@Centril
I've added one more commit actually stabilizing the feature.
Could you start an FCP?

I've been meaning to read this and the other issues / descriptions you wrote but I've been a bit pressed for time. Would you mind waiting a bit (~a day) before I start FCP?

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Dec 27, 2018

@Centril
Sure, a day is okay, we have 3 weeks until the release.

@Centril
Copy link
Contributor

Centril left a comment

Tests, and from what I can see implementation, looks good.

Reassigning for further technical review to:
r? @nikomatsakis

@Centril Centril assigned nikomatsakis and unassigned Centril Dec 28, 2018

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 28, 2018

~A day later...

  • Proposal: treat #[macro_export] macro_rules! { ... } as pub, treat other macro_rules! { ... } as pub(crate).
  • Proposal: Allow imports of built-in attributes, but prohibit actually using names imported this way in attribute positions.
  • Proposal: Allow imports of derive helper attributes, but prohibit actually using names imported this way in attribute positions.
  • Proposal: Allow imports of tool modules, but prohibit actually using names imported this way in attribute paths.
  • Some of the errors are artificial and can be relaxed backward-compatibly in the future, making import paths even closer to non-import paths.

This seems like the right incremental balance for the time being. 👍

All in all, let's:

@rfcbot merge


Known deviations of macro paths and 2018 import paths from the "uniform path" model

There's still some future-proofing in place turning imports referring to local variables and generic parameters into errors - 22a13ef, but that future proofing has two holes.

These 2 holes seem quite pathological... but how much work work would it be to fix them?

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Dec 28, 2018

Team member @Centril has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Dec 28, 2018

These 2 holes seem quite pathological... but how much work work would it be to fix them?

The first one is relatively hard, but it only affects macros, not imports.
I'll look at the second one in the next few days, it shouldn't be too hard to fix, I just treated it as low-priority. UPDATE: Done in aa31d43

@Centril Centril added the relnotes label Dec 28, 2018

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 28, 2018

I have a question: From the comments, it seemed like there was some "concern" about stabilizing this in time for the next release, is there any particular reason for that urgency?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 28, 2018

(Not saying I'm opposed, just asking)

bors added a commit that referenced this pull request Jan 11, 2019

Auto merge of #57516 - pietroalbini:beta-rollup, r=pietroalbini
[beta] Rollup backports

Cherry-picked:

* #57355: use the correct supertrait substitution in `object_ty_for_trait`
* #57471: Updated RELEASES.md for 1.32.0

Rolled up:

* #57483: [beta] Uniform path backports
  * c658d73: resolve: Avoid "self-confirming" resolutions in import validation
  * #57160: resolve: Fix an ICE in import validation
  * #56759: Stabilize `uniform_paths`

r? @ghost
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 11, 2019

@petrochenkov r=me post rebase btw

petrochenkov added some commits Dec 9, 2018

resolve: Prohibit use of uniform paths in macros originating from 201…
…5 edition

...while still keeping ambiguity errors future-proofing for uniform paths.
This corner case is not going to be stabilized for 1.32 and needs some more general experiments about retrofitting 2018 import rules to 2015 edition
Fix a hole in generic parameter import future-proofing
Add some tests for buggy derive helpers

@petrochenkov petrochenkov force-pushed the petrochenkov:prestabuni branch from aa31d43 to 250935d Jan 12, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 12, 2019

@bors r=nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 12, 2019

📌 Commit 250935d has been approved by nikomatsakis

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 12, 2019

@bors p=1
(Needs to get into the upcoming beta.)

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 12, 2019

⌛️ Testing commit 250935d with merge 1bcbb2c...

bors added a commit that referenced this pull request Jan 12, 2019

Auto merge of #56759 - petrochenkov:prestabuni, r=nikomatsakis
Stabilize `uniform_paths`

Address all the things described in #56417.

Assign visibilities to `macro_rules` items - `pub` to `macro_export`-ed macros and `pub(crate)` to non-exported macros, these visibilities determine how far these macros can be reexported with `use`.

Prohibit use of reexported inert attributes and "tool modules", after renaming (e.g. `use inline as imported_inline`) their respective tools and even compiler passes won't be able to recognize and properly check them.

Also turn use of uniform paths in 2015 macros into an error, I'd prefer to neither remove nor stabilize them right away because I still want to make some experiments in this area (uniform path fallback was added to 2015 macros used on 2018 edition in #56053 (comment)).

UPDATE: The last commit also stabilizes the feature `uniform_paths`.

Closes #53130
Closes #55618
Closes #56326
Closes #56398
Closes #56417
Closes #56821
Closes #57252
Closes #57422
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 12, 2019

💔 Test failed - checks-travis

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 12, 2019

The job dist-x86_64-apple of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[02:50:18] travis_time:end:stage2-cargo-miri:start=1547312941492659000,finish=1547312942099053000,duration=606394000

[02:50:18] [TIMING] ToolBuild { compiler: Compiler { stage: 2, host: "x86_64-apple-darwin" }, target: "x86_64-apple-darwin", tool: "cargo-miri", path: "src/tools/miri", mode: ToolRustc, is_optional_tool: true, source_type: Submodule, extra_features: [] } -- 0.637
[02:50:19] [TIMING] Miri { stage: 2, target: "x86_64-apple-darwin" } -- 1.472
The job exceeded the maximum time limit for jobs, and has been terminated.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 12, 2019

The job exceeded the maximum time limit for jobs, and has been terminated.

Looks spurious.
@bors retry

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 12, 2019

⌛️ Testing commit 250935d with merge 75a369c...

bors added a commit that referenced this pull request Jan 12, 2019

Auto merge of #56759 - petrochenkov:prestabuni, r=nikomatsakis
Stabilize `uniform_paths`

Address all the things described in #56417.

Assign visibilities to `macro_rules` items - `pub` to `macro_export`-ed macros and `pub(crate)` to non-exported macros, these visibilities determine how far these macros can be reexported with `use`.

Prohibit use of reexported inert attributes and "tool modules", after renaming (e.g. `use inline as imported_inline`) their respective tools and even compiler passes won't be able to recognize and properly check them.

Also turn use of uniform paths in 2015 macros into an error, I'd prefer to neither remove nor stabilize them right away because I still want to make some experiments in this area (uniform path fallback was added to 2015 macros used on 2018 edition in #56053 (comment)).

UPDATE: The last commit also stabilizes the feature `uniform_paths`.

Closes #53130
Closes #55618
Closes #56326
Closes #56398
Closes #56417
Closes #56821
Closes #57252
Closes #57422
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 12, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: nikomatsakis
Pushing 75a369c to master...

@bors bors merged commit 250935d into rust-lang:master Jan 12, 2019

1 check passed

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