rustc_resolve: refactor NameBindings and fix issue #4952

[jseyfried]

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]

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]

r? @nikomatsakis

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

[nikomatsakis]

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]

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

[nikomatsakis]

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?

[jseyfried]

(edited) see my comment in the main conversation

[bors]

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

[jseyfried]

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

[bors]

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

[jseyfried]

Rebased

[jseyfried]

@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 Defd, 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.

[nikomatsakis]

@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]

@nikomatsakis Done

[nikomatsakis]

Creative use of Index btw.

[nikomatsakis]

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?

[jseyfried]

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]

@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]

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

Thanks @jseyfried, this is nice!

[nikomatsakis]

@jseyfried no.

[nikomatsakis]

@bors r-

[nikomatsakis]

@bors r+

[bors]

📌 Commit 6a6e1db has been approved by nikomatsakis

[bors]

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

[bors]

☀️ 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]

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]

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]

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]

@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]

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]

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

[retep998]

@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]

@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]

@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]

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]

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

[brson]

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

[brson]

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

[brson]

@brson@bluss suggests #21546 describes it.