Correctly walk import lists in AST visitors #28364

petrochenkov · 2015-09-11T22:09:04Z

brson · 2015-09-11T23:01:34Z

Awesome fix - look at all that fallout!

eefriedman · 2015-09-11T23:25:52Z

src/librustc_front/visit.rs

+                                    identifier: name,
+                                    parameters: PathParameters::none()
+                                });
+                                visitor.visit_path(unsafe{&*(&path as *const _)}, id);


visit_path is defined as fn visit_path(&mut self, path: &'v Path, _id: NodeId) {, i.e. the lifetime of the path is tied to the the lifetime of the visitor, not the lifetime of the callback. Also, the current PR visits both the path's components and the path as a whole. Also, cloning the prefix is a bunch of extra computation which is unlikely to be useful

Probably the most straightforward thing to do would be to add a method visit_path_list_item(&mut self, prefix: &'v Path, item: &'v PathListItem) to Visitor and let the callee do the right thing.

Related issue: we probably also want a method on Visitor to visit the prefix; it would be useful for fixing the handling of use std::foobar::{};.

Yes, the prefix is cloned and components of the prefix are visited repeatedly. I supposed this couldn't be done optimally without introducing a new visit_xxx method, but introducing it seemed.. intrusive and too far away from stability.rs. I'll surely do it if it's acceptable.

I also had an idea to desugar use a::{b, c}; into use a::b; use a::b; during lowering to HIR, it would avoid repeated cloning of the prefix, but checks of the prefix components would still be duplicated.

The way the visitor is designed is so that certain visitors can "opt in" to observe the original AST/HIR lifetime and be able to hold onto those references.
Unsafe code here is just wrong.
You have two options: if the HIR map builder doesn't override this particular method, you can drop the lifetime.
Otherwise, you'll have go the explicit route @eefriedman mentioned, with one extra tip: you can always write some sort of iterator for the expansions.

petrochenkov · 2015-09-12T14:52:30Z

Updated with accordance to the @eefriedman 's advice and rebased.
I'm going to seat on this patch a bit more and fix another bug similar to #28075, allowing unstable items outside of braces in imports like

use std::rt::{};

eddyb · 2015-09-12T15:04:05Z

src/libsyntax/visit.rs

                        }
+                    } else {
+                        // FIXME: uncomment this and fix the resulting ICE
+                        // visitor.visit_path(prefix, item.id);


Where does the ICE come from?

Privacy checker. It assumes the prefix is not empty.

bluss · 2015-09-12T17:52:02Z

@brson This bug was discovered when we removed a gated function. With this fix, we add new breaking changes by disallowing imports of still present, unused, unstable functionality. Maybe it deserves a crater run? If the fallout is bad, maybe even a warning cycle (even if that sounds extreme).

petrochenkov · 2015-09-13T12:43:09Z

I'm going to seat on this patch a bit more and fix another bug similar to #28075, allowing unstable items outside of braces in imports like use std::rt::{};

So, fixing this issue would likely require going to librustc_resolve, resolving the prefix and assigning PathResolution to it without actually importing std::rt in the process. I'm not familiar with resolve, so I'm just going to fill a new issue and leave it for later. Update for this PR will follow soon either.

petrochenkov · 2015-09-13T13:26:29Z

Done.

bors · 2015-09-13T21:31:58Z

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

petrochenkov · 2015-09-13T22:26:09Z

Rebased.

eddyb · 2015-09-14T01:13:59Z

@brson Can we get a crater run?

alexcrichton · 2015-09-14T16:07:53Z

I've scheduled crater runs.

alexcrichton · 2015-09-14T21:31:11Z

Looks like this has some relatively significant breakage.

The cause of all the root regressions appears to be that we have an unstable std::intrinsics module, but stable contents (because they're reexported). It was assumed that use std::intrinsics would be guarded, but this bug let it slide, so all these use cases got pass the feature gater.

I would personally think, however, that we could submit PRs to update all those crates, rebuild the other non-root-regressions to make sure there's no more fallout, and then land this while publishing information about how to migrate. The breakage here may be largely limited to the std::intrinsics module, so it may not be that bad.

thoughts @brson?

petrochenkov · 2015-09-15T21:52:55Z

Updated with a fix for #28388

bors · 2015-09-16T02:22:46Z

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

petrochenkov · 2015-09-16T08:04:00Z

Rebased.

brson · 2015-09-16T22:47:05Z

Yeah, let's submit PR's downstream. I can look into it tomorrow.

bors · 2015-09-17T04:53:41Z

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

petrochenkov · 2015-09-17T12:20:36Z

Rebased.

alexcrichton · 2015-09-18T16:21:10Z

I have submitted PRs upstream to affected crates:

alexcrichton · 2015-09-18T16:22:33Z

src/test/compile-fail/issue-28388-3.rs

+
+// Prefix in imports with empty braces should be resolved and checked privacy, stability, etc.
+
+use std::rt::{}; //~ ERROR use of unstable library feature 'rt'


Can you tweak this test to use an aux-build with an unstable module instead? This module in theory may be removed over time.

alexcrichton · 2015-09-18T16:25:32Z

Once those PRs have been merged/released I think we'll want to do another crater run here now that more bugs have been fixed.

petrochenkov · 2015-09-19T09:17:57Z

@alexcrichton
Tests updated.

alexcrichton · 2015-09-19T18:34:43Z

Ok, a new crater run indicates that this has 4 root regressions, 2 of which were spurious because of timeouts and 2 of which were because libraries are depending on an older version of gfx which did not get the fix in gfx-rs/gfx#803.

@brson, how do you feel about merging? I'm comfortable with the breakage here and I think we may want to make a post on internals and such to ensure that everyone knows about this, but I think this should be good to go.

brson · 2015-09-21T19:55:03Z

@alexcrichton Yes, I feel ok about it. Thanks for doing the downstream work.

alexcrichton · 2015-09-21T23:17:43Z

@bors: r+ b44cb01

bors · 2015-09-22T01:12:28Z

⌛ Testing commit b44cb01 with merge e9d2587...

@eddyb

Closes #28075 Closes #28388 r? @eddyb cc @brson

bors · 2015-09-22T03:35:54Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

@nrc

Some minor parts of AST and HIR were not visited by the `visit::walk_xxx` methods - some identifiers, lifetimes, loop labels, attributes of exported macros - but nothing as serious as in, for example, #28364. \+ Added a convenience macro for visiting lists (including Options) \+ Removed some pre-Deref-coersions `&**` noise from visitors r? @nrc

bombless · 2015-12-12T04:42:30Z

mod some_module {
    enum _Enum { A, B, _Dummy }
    pub type Enum = self::_Enum;
    pub use self::_Enum::{A, B};
}
fn to_string(e: some_module::Enum) -> String {
    match e {
        some_module::A => 'A'.to_string(),
        some_module::B => 'B'.to_string(),
        _ => "_not_supported".to_string()
    }
}

We might need to apply the privacy rule to type as well .

EDIT: I checked the PR again and find that above problem is sort of unrelated.

rust-highfive assigned eddyb Sep 11, 2015

brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 11, 2015

eefriedman reviewed Sep 11, 2015
View reviewed changes

petrochenkov force-pushed the usegate branch from 8f62647 to ac660cd Compare September 12, 2015 14:48

petrochenkov changed the title ~~[WIP] Correctly walk import lists in the default AST visitor~~ Correctly walk import lists in AST visitors Sep 12, 2015

eddyb reviewed Sep 12, 2015
View reviewed changes

petrochenkov mentioned this pull request Sep 13, 2015

Privacy, stability etc. are not respected in imports with empty braces #28388

Closed

petrochenkov force-pushed the usegate branch from a57386c to a69a520 Compare September 13, 2015 22:25

petrochenkov force-pushed the usegate branch from a69a520 to d60c936 Compare September 15, 2015 21:51

petrochenkov force-pushed the usegate branch from d60c936 to 355daea Compare September 16, 2015 08:03

petrochenkov added 4 commits September 17, 2015 14:13

Correctly walk import lists in AST visitors

50e42ea

Workaround for imports with empty braces

357982f

Resolve prefix in imports with empty braces

c3f53d1

Fix code broken by rebase

1599c16

petrochenkov force-pushed the usegate branch from 355daea to 1599c16 Compare September 17, 2015 12:19

alexcrichton reviewed Sep 18, 2015
View reviewed changes

alexcrichton mentioned this pull request Sep 18, 2015

Avoid using the std::intrinsics module gfx-rs/gfx#803

Merged

Don't use std in stability tests

b44cb01

bors added a commit that referenced this pull request Sep 22, 2015

Auto merge of #28364 - petrochenkov:usegate, r=alexcrichton

e9d2587

Closes #28075 Closes #28388 r? @eddyb cc @brson

bors merged commit b44cb01 into rust-lang:master Sep 22, 2015

petrochenkov deleted the usegate branch September 23, 2015 12:28

petrochenkov mentioned this pull request Sep 28, 2015

Fill in some missing parts in the default AST and HIR visitors #28715

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly walk import lists in AST visitors #28364

Correctly walk import lists in AST visitors #28364

petrochenkov commented Sep 11, 2015

brson commented Sep 11, 2015

eefriedman Sep 11, 2015

petrochenkov Sep 12, 2015

eddyb Sep 12, 2015

petrochenkov commented Sep 12, 2015

eddyb Sep 12, 2015

petrochenkov Sep 12, 2015

bluss commented Sep 12, 2015

petrochenkov commented Sep 13, 2015

petrochenkov commented Sep 13, 2015

bors commented Sep 13, 2015

petrochenkov commented Sep 13, 2015

eddyb commented Sep 14, 2015

alexcrichton commented Sep 14, 2015

alexcrichton commented Sep 14, 2015

petrochenkov commented Sep 15, 2015

bors commented Sep 16, 2015

petrochenkov commented Sep 16, 2015

brson commented Sep 16, 2015

bors commented Sep 17, 2015

petrochenkov commented Sep 17, 2015

alexcrichton commented Sep 18, 2015

alexcrichton Sep 18, 2015

alexcrichton commented Sep 18, 2015

petrochenkov commented Sep 19, 2015

alexcrichton commented Sep 19, 2015

brson commented Sep 21, 2015

alexcrichton commented Sep 21, 2015

bors commented Sep 22, 2015

bors commented Sep 22, 2015

bombless commented Dec 12, 2015


		// Prefix in imports with empty braces should be resolved and checked privacy, stability, etc.

		use std::rt::{}; //~ ERROR use of unstable library feature 'rt'

Correctly walk import lists in AST visitors #28364

Correctly walk import lists in AST visitors #28364

Conversation

petrochenkov commented Sep 11, 2015

brson commented Sep 11, 2015

eefriedman Sep 11, 2015

Choose a reason for hiding this comment

petrochenkov Sep 12, 2015

Choose a reason for hiding this comment

eddyb Sep 12, 2015

Choose a reason for hiding this comment

petrochenkov commented Sep 12, 2015

eddyb Sep 12, 2015

Choose a reason for hiding this comment

petrochenkov Sep 12, 2015

Choose a reason for hiding this comment

bluss commented Sep 12, 2015

petrochenkov commented Sep 13, 2015

petrochenkov commented Sep 13, 2015

bors commented Sep 13, 2015

petrochenkov commented Sep 13, 2015

eddyb commented Sep 14, 2015

alexcrichton commented Sep 14, 2015

alexcrichton commented Sep 14, 2015

petrochenkov commented Sep 15, 2015

bors commented Sep 16, 2015

petrochenkov commented Sep 16, 2015

brson commented Sep 16, 2015

bors commented Sep 17, 2015

petrochenkov commented Sep 17, 2015

alexcrichton commented Sep 18, 2015

alexcrichton Sep 18, 2015

Choose a reason for hiding this comment

alexcrichton commented Sep 18, 2015

petrochenkov commented Sep 19, 2015

alexcrichton commented Sep 19, 2015

brson commented Sep 21, 2015

alexcrichton commented Sep 21, 2015

bors commented Sep 22, 2015

bors commented Sep 22, 2015

bombless commented Dec 12, 2015