resolve: improve import resolution by jseyfried · Pull Request #31461 · rust-lang/rust

jseyfried · 2016-02-07T08:11:56Z

This PR adds to NameBinding so it can more fully represent bindings from imports as well from items, refactors away Target, generalizes ImportResolution to a simpler type NameResolution, and uses a single NameResolution-valued map in place the existing maps children and import_resolutions (of NameBindings and ImportResolutions, respectively), simplifying duplicate checking and name resolution.

It also unifies the resolve_name_in_module in lib.rs with its namesake in resolve_imports.rs, clarifying and improving the core logic (fixes#31403 and fixes#31404) while maintaining clear future-comparability with shadowable globs (i.e., never reporting that a resolution is a Success or is Failing unless this would also be knowable with shadowable globs).

Since it fixes#31403, this is technically a [breaking-change], but it is exceedingly unlikely to cause breakage in practice. The following is an example of code that would break:

mod foo {pubmod bar {}// This defines bar in the type namespacepubuse alpha::bar;// This defines bar in the value namespace// This should define baz in both namespaces, but it only defines baz in the type namespace.pubuseself::bar as baz;pubfnbaz(){}// This should collide with baz, but now it does not.}pubfnf(){}mod alpha {pubuseself::f as bar;// Changing this to `pub fn bar() {}` causes the collision right now.pubusesuper::*;}

r? @nrc

jseyfried · 2016-02-07T08:20:46Z

cc @petrochenkov @nikomatsakis

petrochenkov · 2016-02-07T11:05:18Z

single NameResolution-valued map in place the existing maps children and import_resolutions

🎉

fixes #31403 and fixes #31403

The issue numbers are the same.

I'll read this today.

jseyfried · 2016-02-07T11:16:33Z

The issue numbers are the same.

The second one was supposed to be #31404, edited.

I'll read this today.

Awesome, I look forward to any feedback.

petrochenkov · 2016-02-07T17:22:37Z

Ok, I give up, too many things are happening here at the same time.
I'm happy with the direction of unifying use items with other items, but can't follow many concrete changes, especially that I'm not very familiar with the import resolution code.

By the way, how much of the code treating imports and extern crates specially is caused only by error message compatibility?

petrochenkov · 2016-02-07T19:00:26Z

So, if we try to define a shadowable binding (1) conflicting with another existing shadowable binding (2), then (2) wins or the conflict is reported somewhere else?

Yeah, binding (2) would win (since the first match arm would be taken) and the conflict would not be reported (it would also not be reported in the original code). This isn't a problem since the only SHADOWABLE bindings are from the special prelude import, so there can't be conflicts.

Perhaps I should rename SHADOWABLE to PRELUDE.

Perhaps I should rename SHADOWABLE to PRELUDE.
That would be much clearer, because prelude and shadowable glob imports would behave differently (I suppose).
The resolution result depending on the resolution order is still pretty bad, even if the concern is theoretical at the moment due to absence of user-defined preludes.

True, I'll make it an error to have duplicate SHADOWABLE / PRELUDE imports.

jseyfried · 2016-02-07T19:06:15Z

too many things are happening here at the same time

I'll split the first commit into more smaller commits.

how much of the code treating imports and extern crates specially is caused only by error message compatibility?

The special treatment in report_conflicts is for error message compatibility and the special treatment in record_use is for compatibility with the unused extern crate tracking system (which itself needs some refactoring; it tracks extern crates themselves, and not extern crate items so that if the same extern crate is declarated twice in different modules and only one declaration is used, the unused declaration will not be reported).
Apart from that, the special treatment is a reflection of semantics.

jseyfried · 2016-02-08T01:20:15Z

I addressed @gereeter's and @petrochenkov's comments and rewrote history to make the biggest commit a little cleaner and easier to review.

…es in lib.rs

NameBinding now encodes these directly with binding.is_public() and (binding.is_public() && binding.is_import()) (respectively)

…ame ImportResolution to NameResolution

…tanding_references_for

…valued map. Refactor away resolve_name_in_module in resolve_imports.rs Rewrite and improve the core name resolution procedure in NameResolution::result and Module::resolve_name Refactor the duplicate checking code into NameResolution::try_define

petrochenkov · 2016-02-08T19:36:17Z

Reviewed everything except for jseyfried@7000e70

…sts) Derive the Default impl for NameResolution

nrc · 2016-02-11T02:42:34Z

@bors: r+

bors · 2016-02-11T02:42:35Z

📌 Commit 3df40c0 has been approved by nrc

bors · 2016-02-11T04:27:14Z

⌛ Testing commit 3df40c0 with merge 1de70d3...

@nrc

This PR adds to `NameBinding` so it can more fully represent bindings from imports as well from items, refactors away `Target`, generalizes `ImportResolution` to a simpler type `NameResolution`, and uses a single `NameResolution`-valued map in place the existing maps `children` and `import_resolutions` (of `NameBinding`s and `ImportResolution`s, respectively), simplifying duplicate checking and name resolution. It also unifies the `resolve_name_in_module` in `lib.rs` with its namesake in `resolve_imports.rs`, clarifying and improving the core logic (fixes#31403 and fixes#31404) while maintaining clear future-comparability with shadowable globs (i.e., never reporting that a resolution is a `Success` or is `Failing` unless this would also be knowable with shadowable globs). Since it fixes#31403, this is technically a [breaking-change], but it is exceedingly unlikely to cause breakage in practice. The following is an example of code that would break: ```rust mod foo { pub mod bar {} // This defines bar in the type namespace pub use alpha::bar; // This defines bar in the value namespace // This should define baz in both namespaces, but it only defines baz in the type namespace. pub use self::bar as baz; pub fn baz() {} // This should collide with baz, but now it does not. } pub fn f() {} mod alpha { pub use self::f as bar; // Changing this to `pub fn bar() {}` causes the collision right now. pub use super::*; } ``` r? @nrc

jseyfried · 2016-02-11T05:03:39Z

@nrc Thanks!

bors · 2016-02-11T06:48:45Z

☀️ 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-mac-ios-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

…d to catch it.

@nrc

This fixes a regression caused by #31461 allowing extern crates to be glob imported, and it fixes the test that was supposed to catch it. r? @nrc

@nrc

Now that #31461 is merged, a failing resolution can never become indeterminate or succeed, so we no longer have to keep trying to resolve failing import directives. r? @nrc

jseyfried force-pushed the remove_import_resolutions branch from 828bd48 to 6a2f81eCompare February 7, 2016 08:18

jseyfried force-pushed the remove_import_resolutions branch from 6a2f81e to eb1a9b6Compare February 7, 2016 10:37

petrochenkov reviewed Feb 7, 2016
View reviewed changes

jseyfried force-pushed the remove_import_resolutions branch from eb1a9b6 to fe6ecd7Compare February 8, 2016 01:11

rust-highfive assigned nrc Feb 8, 2016

jseyfried force-pushed the remove_import_resolutions branch 2 times, most recently from 85afd8e to a847317Compare February 8, 2016 02:13

jseyfried added 11 commits February 8, 2016 02:21

Make resolve_name_in_module solely responsible for tracking used crat…
661b7ce
…es in lib.rs

Refactor away Target
7366d10

Add and use an arena for NameBindings
22e189e

Expand NameBinding to better represent bindings from imports
2e24c74

Refactor away separate tracking of used_public and used_reexport.
4428b1c
NameBinding now encodes these directly with binding.is_public() and (binding.is_public() && binding.is_import()) (respectively)

Refactor away the fields id and is_public of ImportResolution and ren…
96b4dc4
…ame ImportResolution to NameResolution

Write and use increment_outstanding_references_for and decrement_outs…
16e7ff1
…tanding_references_for

Change try_define_child to return a Result instead of an Option
d881eee

Refactor away add_export and cleanup the end of resolve_single_import
d7734ae

Reallow methods from traits that are shadowed by non-import items
3c62d90

jseyfried force-pushed the remove_import_resolutions branch from a847317 to 3c62d90Compare February 8, 2016 02:25

jseyfried force-pushed the remove_import_resolutions branch from 69635f0 to 3c62d90Compare February 9, 2016 04:07

Allow prelude imports to shadow eachother (needed for the [pretty] te…
3df40c0
…sts) Derive the Default impl for NameResolution

jseyfried force-pushed the remove_import_resolutions branch from 1e71fc0 to 3df40c0Compare February 9, 2016 17:33

bors merged commit 3df40c0 into rust-lang:masterFeb 11, 2016

jseyfried added a commit to jseyfried/rust that referenced this pull request Feb 11, 2016
Fix regression fromrust-lang#31461and fix the test that was suppose…
9189463
…d to catch it.

jseyfried mentioned this pull request Feb 11, 2016
Fix regression in resolve #31566
Merged

bors added a commit that referenced this pull request Feb 15, 2016
Auto merge of#31566- jseyfried:fix_regression, r=nrc
69ad912
This fixes a regression caused by #31461 allowing extern crates to be glob imported, and it fixes the test that was supposed to catch it. r? @nrc

jseyfried mentioned this pull request Feb 18, 2016
Resolve: stop trying to resolve an import directive after the resolution fails #31747
Merged

jseyfried deleted the remove_import_resolutions branch March 25, 2016 22:54

jseyfried mentioned this pull request May 14, 2016
Name resolution has changed between 1.7.0 and 1.8.0 #33458
Closed

jseyfried changed the title ~~Resolve: refactor away Module::import_resolutions and improve import resolution~~resolve: improve import resolutionFeb 14, 2017

Uh oh!

Uh oh!

Conversation

jseyfried commented Feb 7, 2016

Uh oh!

jseyfried commented Feb 7, 2016

Uh oh!

petrochenkov commented Feb 7, 2016

Uh oh!

jseyfried commented Feb 7, 2016

Uh oh!

petrochenkov commented Feb 7, 2016

Uh oh!

petrochenkovFeb 7, 2016

Choose a reason for hiding this comment

Uh oh!

jseyfriedFeb 7, 2016

Choose a reason for hiding this comment

Uh oh!

jseyfriedFeb 7, 2016

Choose a reason for hiding this comment

Uh oh!

petrochenkovFeb 7, 2016

Choose a reason for hiding this comment

Uh oh!

jseyfriedFeb 7, 2016

Choose a reason for hiding this comment

Uh oh!

jseyfried commented Feb 7, 2016

Uh oh!

jseyfried commented Feb 8, 2016

Uh oh!

petrochenkov commented Feb 8, 2016

Uh oh!

nrc commented Feb 11, 2016

Uh oh!

bors commented Feb 11, 2016

Uh oh!

bors commented Feb 11, 2016

Uh oh!

jseyfried commented Feb 11, 2016

Uh oh!

bors commented Feb 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants