rustc_resolve: refactor NameBindings and fix issue #4952 #29530

jseyfried · 2015-11-03T00:04:40Z

Replace TypeNsDef and ValueNsDef with a more general type NsDef.

Define a newtype NameBinding for Rc<RefCell<Option<NsDef>>> and refactor NameBindings to be a NameBinding for each namespace.

Replace uses of NameBindings with NameBinding where only one binding is being used (in NamespaceResult, Target, etc).

Refactor away resolve_definition_of_name_in_module and NameDefinition, fixing issue #4952.

rust-highfive · 2015-11-03T00:05:00Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Aatch (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

nikomatsakis · 2015-11-03T09:35:39Z

r? @nikomatsakis

(I saw @Aatch mention on IRC that he did not feel familiar enough with resolve to review)

nikomatsakis · 2015-11-11T20:28:16Z

Sorry for being slow. I'll try to take a look at this now. I'm also not super familiar with this code, but I figure this is a good time to refresh my memory. :)

nikomatsakis · 2015-11-11T20:42:48Z

@jseyfried to be clear, this is a pure refactoring, right? That is, it's not supposed to fix any bugs?

nikomatsakis · 2015-11-11T20:45:12Z

src/librustc_resolve/lib.rs

-    type_def: Option<Def>,
-    type_span: Option<Span>
+    def: Option<Def>,
+    module: Option<Rc<Module>>,


Some comments here would be good. e.g. what does it mean if both def and module are Some? The function def below seems to suggest that def "takes precedence" -- does this mean that module should always be None when def is Some?

(edited) see my comment in the main conversation

bors · 2015-11-11T22:28:17Z

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

jseyfried · 2015-11-14T09:16:12Z

@nikomatsakis Thanks for the feedback. Yes, this is a pure refactoring.

bors · 2015-11-16T00:03:27Z

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

Define a newtype `NameBinding` for `Rc<RefCell<Option<NsDef>>>` and refactor `NameBindings` to be a `NameBinding` for each namespace. Replace uses of `NameBindings` with `NameBinding` where only one binding is being used (in `NamespaceResult`, `Target,` etc). Refactor away `resolve_definition_of_name_in_module` and `NameDefinition`.

jseyfried · 2015-11-16T07:39:39Z

Rebased

jseyfried · 2015-11-16T07:53:18Z

@nikomatsakis Regarding your question about the def and module fields of NsDef:

When I was writing the first commit, I wasn't exactly sure what it meant when the type_def and module_def fields of a TypeNsDef were both Some, so I defined NsDef so that if it was representing what used to be a TypeNsDef, its def and module fields would mean the same thing as the type_def and module_def fields of the TypeNsDef. I gave the def field precedence in NsDef::def() (and hence in Namebinding::def()) since the type_def field was given precedence in Namebindings::def_for_namespace().

It turns out the meaning of these fields is fairly complex.

If the NsDef is representing a value, struct, enum variant, or associated type definition with Def d, def is Some(d) and module is None.
If the NsDef is representing a (possible foreign) module definition d, def is None and module is Some(mod), where mod.kind is NormalModuleKind and mod.def_id is Some(d.def_id()).
If the NsDef is representing an enum, trait, or type synonym definition d, def is Some(d) and module is Some(mod), where mod.kind is EnumModuleKind, TraitModuleKind, or TypeModuleKind (respectively) and mod.def_id is Some(d.def_id()).
If the NsDef is representing a block, def is None and module is Some(mod), where mod.kind is AnonymousModuleKind and mod.def_id is None.

The kind and def_id fields of Module are encoding slightly less information that the Option<Def> returned by NsDef::def() (if kind is TypeModuleKind, then the Def could be DefTy(def_id, true) or DefTy(def_id, false)), so at least in the case of a type synonym we need both the def and module fields to be Some.

This redundancy seems like a bad idea, so I refactored the kind and def_id fields of Module into a single field def: Cell<Option<Def>> (kind and def_id were Cell<ModuleKind> and Cell<Option<DefId>>, respectively) and changed build_reduced_graph.rs to only set the module field where it had previously set both fields.

Thus ideally exactly one of def and module are Some at any point and they can be refactored into something like Result<Def, Rc<Module>>.

However, we sometimes allow struct and mod declarations to collide (see issue #21546 and merged PR #26421, which makes this a warning) due to an easily fixable bug in the duplicate checking code. When this happens, the def field corresponds to the struct declaration and the module field corresponds to the mod declaration, and we need both to preserve backward compatability.

It looks like it was decided that there would be one warning cycle before the warnings become errors. Once they are errors, I can refactor the def and module fields into something like Result<Def, Rc<Module>> very easily.

The warnings are currently in the beta branch. Does this mean they are ready to be made errors now in nightly? If so I could do that and refactor the fields in this PR.

Change build_reduced_graph.rs so the fields def and module of NsDef are never both Some unless the NsDef represents a duplicate definition (see issue 26421).

nikomatsakis · 2015-11-16T22:49:48Z

@jseyfried

The warnings are currently in the beta branch. Does this mean they are ready to be made errors now in nightly? If so I could do that and refactor the fields in this PR.

Yes, it does mean that. That'd be nice!

jseyfried · 2015-11-17T04:06:44Z

@nikomatsakis Done

nikomatsakis · 2015-11-17T19:01:57Z

src/librustc_resolve/lib.rs

+    value_ns: NameBinding, // < Meaning in value namespace.
+}
+
+impl ::std::ops::Index<Namespace> for NameBindings {


Creative use of Index btw.

…-liners

nikomatsakis · 2015-11-18T15:09:25Z

src/librustc_resolve/lib.rs

+        self.def.get().as_ref().map(Def::def_id)
+    }
+
+    fn is_normal(&self) -> bool {


can you add a comment explaining what makes this a "normal" module? I guess this means "a module declared by the user", and not a "pseudo-module" that we create for some other reason?

Module probably should be named something like Scope since it represents scopes corresponding to traits, enums, and blocks as well as actual mod modules.
In this case a "normal" Module (formerly NormalModuleKind) is a Module that corresponds to a actual module or a foreign module.

nikomatsakis · 2015-11-18T15:23:22Z

@jseyfried ok, I've read this through. I think it looks really nice, definitely seems cleaner, and I like cleaning up the semantics. r+ if you add a comment on is_normal and explain why the test changed :) (Please comment when you make changes, since GH doesn't notify of new commits otherwise. I'll try to keep my eye out but feel free to ping me on IRC too.)

nikomatsakis · 2015-11-19T10:18:48Z

@bors r+ (I decided insisting on a comment on is_normal was unnecessary.)

Thanks @jseyfried, this is nice!

nikomatsakis · 2015-11-23T15:52:30Z

@jseyfried no.

nikomatsakis · 2015-11-23T15:52:36Z

@bors r-

nikomatsakis · 2015-11-23T15:52:46Z

@bors r+

bors · 2015-11-23T15:52:47Z

📌 Commit 6a6e1db has been approved by nikomatsakis

bors · 2015-11-23T17:31:20Z

⌛ Testing commit 6a6e1db with merge 8e9a975...

Replace `TypeNsDef` and `ValueNsDef` with a more general type `NsDef`. Define a newtype `NameBinding` for `Rc<RefCell<Option<NsDef>>>` and refactor `NameBindings` to be a `NameBinding` for each namespace. Replace uses of `NameBindings` with `NameBinding` where only one binding is being used (in `NamespaceResult`, `Target,` etc). Refactor away `resolve_definition_of_name_in_module` and `NameDefinition`, fixing issue #4952.

bors · 2015-11-23T20:08:47Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, 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

apoelstra · 2015-12-09T06:04:11Z

This breaks rust-jsonrpc and there isn't any workaround short of me breaking my API to force users to pass garbage to my macros. Why wasn't there a warning?

apoelstra · 2015-12-09T06:40:38Z

With help from @bluss I was able to find a workaround that does not change the API.

However, the warning PR and this PR were merged less than four weeks apart. My crate is publicly available on crates.io and has transitioned from "working" to "not compiling" in such a short period, with no notification other than that one of my users who I know IRL happened to be using nightly with my library.

mbrubeck · 2015-12-09T22:26:46Z

Some additional detail from @apoelstra on IRC: This breakage wasn't found by the crater run because the error occurs in a macro expansion, and doesn't happen during cargo build for the jsonrpc crate, only in cargo test (which crater doesn't do).

The code that broke in jsonrpc was using a pattern that may be found in other macros, so there might be other broken crates that were missed by crater for the same reason. Should we try harder to determine the real impact before converting this warning to an error?

nikomatsakis · 2015-12-10T12:02:53Z

@apoelstra @mbrubeck I am confused. I thought that the warnings had been available for some time. Perhaps I misunderstood what @jseyfried meant by "in the beta branch" -- if the warnings were only available for six hours, that does seem like a policy failure, since you ought to have time to react.

mbrubeck · 2015-12-10T13:07:48Z

The warning landed in 1.5 and became an error in 1.6, so developers using the stable channel will have one release cycle (six weeks) to react to them. On the nightly channel, developers had four weeks to react.

mbrubeck · 2015-12-10T13:18:29Z

We could detect this type of breakage in test code by making crater do cargo test --no-run.

retep998 · 2015-12-11T00:12:24Z

@mbrubeck If crater does do cargo test --no-run it should still consider it a partial success if cargo build passes so that cargo test --no-run failing on both runs won't hide a regression in cargo build.

apoelstra · 2015-12-11T02:22:38Z

@nikomatsakis Sorry, I misunderstood how the release cycle worked. I thought that because nightly had a hard error before stable had even a warning, that stable would not get even the warning.

nikomatsakis · 2015-12-14T09:34:23Z

@mbrubeck it seems like I misunderstood. Re-reading the comments, I'm not sure where I got the notion that the span-between-warning-and-breakage was 6 hours. In any case, 4 weeks seems like a pretty reasonable span of time (for Nightly), but I don't think we've made a firm policy here.

Regarding the crater run, perhaps cargo test --no-run might be of interest, there are certainly numerous ways that we could use crater to get more data (cc @brson).

nikomatsakis · 2015-12-14T21:05:52Z

On Thu, Dec 10, 2015 at 06:23:19PM -0800, Andrew Poelstra wrote:

@nikomatsakis Sorry, I misunderstood how the release cycle worked. I thought that because nightly had a hard error before stable had even a warning, that stable would not get even the warning.

Not to worry. Thanks for raising the point in any case, better safe
than sorry.

mbrubeck · 2015-12-15T22:05:32Z

As mentioned by bhsausabsha on IRC, cargo bench --no-run is another possible source of code coverage for crater.

brson · 2016-01-19T18:49:54Z

Are there any other links relevant to the regression reported here? Maybe an issue?

brson · 2016-01-19T18:51:17Z

Since #30089 was fixed, I'm assuming it is a different regression than the one reported by @apoelstra.

brson · 2016-01-19T19:04:59Z

@brson @bluss suggests #21546 describes it.

rust-highfive assigned Aatch Nov 3, 2015

rust-highfive assigned nikomatsakis and unassigned Aatch Nov 3, 2015

nikomatsakis reviewed Nov 11, 2015
View reviewed changes

jseyfried force-pushed the resolve branch from 0072fd1 to b7a3e80 Compare November 14, 2015 08:57

jseyfried force-pushed the resolve branch from b7a3e80 to ceda838 Compare November 16, 2015 03:03

Refactor fields def_id and kind of Module into a single field def.

8a6187f

Change build_reduced_graph.rs so the fields def and module of NsDef are never both Some unless the NsDef represents a duplicate definition (see issue 26421).

jseyfried force-pushed the resolve branch from ea68cd8 to 8a6187f Compare November 16, 2015 08:00

Fix issue rust-lang#21546 and refactor NsDef

572c2f3

jseyfried force-pushed the resolve branch from 96533ba to 572c2f3 Compare November 17, 2015 09:18

nikomatsakis reviewed Nov 17, 2015
View reviewed changes

Refactor away get_module_if_available and get_module and reformat one…

6a6e1db

…-liners

nikomatsakis reviewed Nov 18, 2015
View reviewed changes

bors merged commit 6a6e1db into rust-lang:master Nov 23, 2015

bors mentioned this pull request Nov 23, 2015

privacy: Rewrite VisiblePrivateTypesVisitor #29973

Merged

nrc mentioned this pull request Nov 24, 2015

Inconsistent symbol duplication checking #21546

Closed

jseyfried mentioned this pull request Nov 24, 2015

Possibly merge resolve_definition_of_name_in_module with resolve_name_in_module #4952

Closed

alexcrichton mentioned this pull request Nov 30, 2015

Regression: a module named std has already been imported in this module [E0252] #30089

Closed

jseyfried mentioned this pull request Dec 1, 2015

Fixes #30089 #30133

Merged

bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Dec 9, 2015

brson added the regression-from-stable-to-stable Performance or correctness regression from one stable version to another. label Jan 19, 2016

jseyfried deleted the resolve branch January 31, 2016 02:17

rustc_resolve: refactor NameBindings and fix issue #4952 #29530

rustc_resolve: refactor NameBindings and fix issue #4952 #29530

Conversation

jseyfried commented Nov 3, 2015

rust-highfive commented Nov 3, 2015

nikomatsakis commented Nov 3, 2015

nikomatsakis commented Nov 11, 2015

nikomatsakis commented Nov 11, 2015

nikomatsakis Nov 11, 2015

Choose a reason for hiding this comment

jseyfried Nov 14, 2015

Choose a reason for hiding this comment

bors commented Nov 11, 2015

jseyfried commented Nov 14, 2015

bors commented Nov 16, 2015

jseyfried commented Nov 16, 2015

jseyfried commented Nov 16, 2015

nikomatsakis commented Nov 16, 2015

jseyfried commented Nov 17, 2015

nikomatsakis Nov 17, 2015

Choose a reason for hiding this comment

nikomatsakis Nov 18, 2015

Choose a reason for hiding this comment

jseyfried Nov 19, 2015

Choose a reason for hiding this comment

nikomatsakis commented Nov 18, 2015

nikomatsakis commented Nov 19, 2015

nikomatsakis commented Nov 23, 2015

nikomatsakis commented Nov 23, 2015

nikomatsakis commented Nov 23, 2015

bors commented Nov 23, 2015

bors commented Nov 23, 2015

bors commented Nov 23, 2015

apoelstra commented Dec 9, 2015

apoelstra commented Dec 9, 2015

mbrubeck commented Dec 9, 2015

nikomatsakis commented Dec 10, 2015

mbrubeck commented Dec 10, 2015

mbrubeck commented Dec 10, 2015

retep998 commented Dec 11, 2015

apoelstra commented Dec 11, 2015

nikomatsakis commented Dec 14, 2015

nikomatsakis commented Dec 14, 2015

mbrubeck commented Dec 15, 2015

brson commented Jan 19, 2016

brson commented Jan 19, 2016

brson commented Jan 19, 2016