resolve: re-export ambiguity as warning #114682

bvanjoi · 2023-08-10T03:58:08Z

Expose these ambiguous bindings as warnings instead of simply concealing them. This change introduces potential breaking alterations. For instance, code that previously compiled successfully may now fail to compile as expected:

// lib.rs
mod a {
  pub type C = i8;
}

mod b {
  pub type C = i16;
}

pub use a::*;
pub use b::*;

// main.rs
extern crate lib;

mod a {
    pub type C = i32;
}

use lib::*;
use a::*;

fn main() {
    let _: C = 1; // it will throw an ambiguity error but previous it will not.
}

r? @petrochenkov

compiler/rustc_middle/src/metadata.rs

tests/ui/imports/glob-conflict-cross-crate.rs

compiler/rustc_resolve/src/lib.rs

compiler/rustc_resolve/src/build_reduced_graph.rs

compiler/rustc_resolve/src/diagnostics.rs

compiler/rustc_lint_defs/src/lib.rs

compiler/rustc_middle/src/metadata.rs

bvanjoi · 2023-09-12T16:49:45Z

I will proceed with this PR once #115269 is merged, therefore, it is currently marked as a draft.

petrochenkov · 2023-09-13T05:01:26Z

Our labels are better for tracking this stuff than github's draft status.
Blocked on #115269.
@rustbot blocked

petrochenkov · 2023-10-07T11:15:33Z

#115269 has landed.
@rustbot author

JohnCSimon · 2023-11-12T18:38:58Z

I will proceed with this PR once #115269 is merged, therefore, it is currently marked as a draft.
this has been merged

@bvanjoi

ping from triage - can you post your status on this PR? There hasn't been an update in a few months. Thanks!

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

bvanjoi · 2023-11-13T05:25:01Z

I nearly overlooked this task, but I intend to continue with it within this week.

tests/ui/imports/glob-conflict-cross-crate.rs

tests/ui/imports/glob-conflict-cross-crate-2.rs

tests/ui/imports/glob-conflict-cross-crate-3.rs

bvanjoi · 2023-11-18T17:38:44Z

ci is green @rustbot ready

compiler/rustc_middle/src/metadata.rs

compiler/rustc_metadata/src/rmeta/mod.rs

compiler/rustc_middle/src/query/mod.rs

compiler/rustc_metadata/src/rmeta/decoder.rs

compiler/rustc_errors/src/lib.rs

compiler/rustc_resolve/src/build_reduced_graph.rs

bvanjoi · 2024-01-08T12:01:26Z

It seems these regressions are within our expectations. Should we add this as a lint?

petrochenkov · 2024-01-08T14:35:41Z

Yes, these errors should be reported as a deprecation lint for some time.

bvanjoi · 2024-01-08T18:41:38Z

I've come across an unusual behavior, though I'm unsure if it's a bug:

// extern-crate.rs
mod d {
    pub fn max() {}
    pub(crate) mod max {}
}

mod e {
    pub type max = i32;
}

pub use self::d::*;
pub use self::e::*;

// code.rs
use extern_crate::max; // refer to the fn `extern_crate::d::max`

pub fn main() {}

but if I delete the module of d::max, it will:

// extern-crate.rs
mod d {
    pub fn max() {}
}

mod e {
    pub type max = i32;
}

pub use self::d::*;
pub use self::e::*;

// code.rs
use extern_crate::max; // refers to `extern_crate::e::max`

pub fn main() {}

However, I think they should both refer to extern_crate::e::max. It appears that the visibility influences this behavior.

petrochenkov · 2024-01-08T23:42:45Z

In both examples use extern_crate::max should refer to fn d::max in value namespace and type e::max in type namespace.

The disambiguation between mod max and type max in the first example is questionable though.
I guess, this code is responsible for it https://github.com/rust-lang/rust/blob/master/compiler/rustc_resolve/src/imports.rs#L332-L334.
The intent is understandable, but it's not really correct, and breaks the resolution determinacy logic.

bvanjoi · 2024-01-09T06:58:20Z

I've submitted a patch to convert this issue into a warning.

At the moment, I think there are still some regressions:

For polywrap_xxx, the Result is defined in the std::predule, which is cumbersome to handle. Consequently, I've decided to leave this as is.
As for manage_relay_users, it's not a widely used library, and we can fix this regression easily, thus I think it's unnecessary to complicate the logic further.
Regarding diesel@1.x: It presents the majority of regression cases and manages some challenging situations (as seen in the above case). Fortunately, diesel@1.x, which was released two years ago, is progressively being phased out and replaced by diesel@2.x.

Do you think it would be viable for us to submit PRs for them, instead of introducing them as lint?

bvanjoi · 2024-01-10T14:48:24Z

It appears that the author of diesel is not in favor of applying the patch(diesel-rs/diesel#3898 (comment)), so we need to revisit this issue..

Ten0 · 2024-01-12T13:55:44Z

Hey,

There is some ongoing discussion in Diesel about specifically the design around this.

According to the original comment above, this appears to be about conflicting type aliases that resolve to different types.
IIUC this PR reports as conflicts type aliases and modules that have the same name, which is definitely a better behavior than ignoring them.

I'm not sure I understand if this PR also considers as conflicts:

Functions and type aliases (currently these inter-operate well and the dsl::auto_type feature seems to highlight a use-case where it may be practical to not have that behavior create a warning at every use).
Type aliases that resolve to the same underlying type (that are written in the exact same way besides path)
- unless I'm mistaken, besides the naming overlaps that we're currently working on fixing, such "conflicts" due to glob imports in Diesel should always be restricted to cases where the types actually resolve to the same underlying type (once all type aliasing is resolved).
Functions and functions that resolve to the same underlying function

Thanks,

bors · 2024-01-13T06:41:30Z

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

petrochenkov · 2024-01-17T13:51:07Z

@bvanjoi
As a last resort, it's always possible to add a workaround tailored to a specific crate (diesel-1.4.* in this case).
You can look at fn pretty_printing_compatibility_hack for an example, which also added a similar workaround for proc-macro-hack/procedural-masquerade crates that caused massive regressions after an important compiler bugfix.

petrochenkov · 2024-01-17T13:56:27Z

@bvanjoi
Could you submit all the test suite additions/renamings/etc from this PR as a separate PR to make a snapshot of the current compiler behavior for all these cases, before the fixes?
Landing the fixes may take some time, but tests we can land now.

petrochenkov · 2024-01-17T14:23:29Z

compiler/rustc_resolve/src/build_reduced_graph.rs

+                .build_reduced_graph_for_external_crate_res(child, false);
+        }
+        let children = self.cstore().ambiguity_module_children_untracked(def_id, self.tcx.sess);
+        for child in &children {


Suggested change

for child in &children {

for child in &self.cstore().ambiguity_module_children_untracked(def_id, self.tcx.sess) {

petrochenkov · 2024-01-17T14:23:57Z

compiler/rustc_resolve/src/build_reduced_graph.rs

+        }
+        let children = self.cstore().ambiguity_module_children_untracked(def_id, self.tcx.sess);
+        for child in &children {
+            let parent_scope = ParentScope::module(module, self);


This line is common for both loops and can be moved out of them, if borrow checker allows.

petrochenkov · 2024-01-17T14:42:05Z

compiler/rustc_resolve/src/imports.rs

@@ -323,7 +324,10 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                            // imported from the same glob-import statement.
                            resolution.binding = Some(binding);
                        } else if res != old_binding.res() {
-                            let binding = if warn_ambiguity {
+                            let binding = if old_binding.is_ambiguity_in_extern_crate() {


What if the new binding is an ambiguity in extern crate?
In that case we should probably report a warning instead of an error too.

petrochenkov · 2024-01-17T14:43:12Z

compiler/rustc_resolve/src/imports.rs

-
-                    children.push(ModChild { ident, res, vis: binding.vis, reexport_chain });
+                    let ambiguity = binding.is_ambiguity() && !binding.warn_ambiguity;
+                    if ambiguity {


Suggested change

if ambiguity {

if binding.is_ambiguity() && !binding.warn_ambiguity {

weiznich · 2024-01-18T07:30:58Z

It appears that the author of diesel is not in favor of applying the patch(diesel-rs/diesel#3898 (comment)), so we need to revisit this issue..

It feels like I should explain that a bit more in detail. One major point for not wanting to release another diesel 1.4.x patch release is that it is essentially unsupported since nearly 2 years now. I fear that publishing another release will lead to the impression that it is still supported and might raise questions why certain other fixes/features are not backported. That written: I'm not 100% opposed to provide a patch release, it's just something that I see as last resort if there are really no other possibilities.

The other important point to note is: As it is unsupported for nearly two years and it seems likely that most user that continues to use it just not apply updates at all. That means releasing another patch update will likely not address the breakage there as there is little evidence that this patch release will be used by the broken crates.

Would it be possible to have this as future-incompatible lint that warns for a few stable releases about that breakage and points to an issue so that we can gather some evidence if someone would actually care? Lib.rs still lists 28k weekly downloads for diesel 1.4.8 so there seem to exist quite a few potential users out there.

(Also it would be great to have an answer to #114682 (comment) to be certain that diesel 2.x is not affected by this)

bvanjoi · 2024-01-31T13:22:51Z

IIUC this PR reports as conflicts type aliases and modules that have the same name

To be precise, it simply aims to maintain ambiguity for the ambiguous items in the external crate. This won't affect any interactions between different namespaces:

for example:

// extern-crate.rs
mod a {
    pub type A = i32;
}

mod b {
   pub type A = u32;
}

pub use a::*;
pub use b::*;

// code.rs
use extern_crate::A; 
const C: A = 1;//`A` will become an ambiguous item if the PR is merged.

but:

// extern-crate.rs
mod a {
    pub type A = i32;
}

mod b {
   pub const A: u8 = 0;
}

pub use a::*;
pub use b::*;

// code.rs
use extern_crate::A;
const C: A = 1; // this case can still compile successfully.

bvanjoi · 2024-01-31T13:40:22Z

Would it be possible to have this as future-incompatible lint that warns for a few stable releases about that breakage and points to an issue

I used to consider doing this as well, but the regression report indicated that migrations_internals was the only area significantly affected. This particular regression case led me to question the worth of introducing complex linting logic into rustc, as opposed to fixing this case directly. Hence, the reason why I opened a PR for diesel@1.4.x.

weiznich · 2024-02-02T13:52:12Z

I used to consider doing this as well, but the regression report indicated that migrations_internals was the only area significantly affected.

My main concern is that while your PR to diesel fixes this particular regression, it does not fix the underlying issue that dsl::max (and some other methods like min/sum/…) create these ambiguous items. The problem with that is that any usage of use diesel::dsl::max in any other crate will become ambiguous by that as far as I understand. I'm reasonable sure that this will affect other code than diesel_migrations as that's how we documented to use these functions. There will certainly be uses of that in non-public code bases as well. That's something that also will affect diesel 2.0 users as the code that does this is still there, although that might change with the next feature release.

bvanjoi · 2024-02-02T14:03:09Z

My main concern is that while your PR to diesel fixes this particular regression, it does not fix the underlying issue that dsl::max (and some other methods like min/sum/…) create these ambiguous items.

Yes, I agree that this is a reasonable conclusion. There have always been some scenarios that were not considered.

That's something that also will affect diesel 2.0 users as the code that does this is still there

I don't think this is entirely correct, given that diesel@2 has successfully compiled in the past

JohnCSimon · 2024-04-14T16:55:29Z

@bvanjoi

Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you are going to continue please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

rustbot assigned petrochenkov Aug 10, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 10, 2023

bvanjoi force-pushed the fix-36837 branch from fd75e06 to 626c193 Compare August 10, 2023 04:00

bvanjoi commented Aug 10, 2023

View reviewed changes

compiler/rustc_middle/src/metadata.rs Outdated Show resolved Hide resolved

petrochenkov reviewed Aug 16, 2023

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 16, 2023

bvanjoi mentioned this pull request Sep 12, 2023

regression: import resolution ambiguity #115777

Open

bvanjoi marked this pull request as draft September 12, 2023 16:50

rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 13, 2023

petrochenkov marked this pull request as ready for review September 13, 2023 05:01

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Oct 7, 2023

bvanjoi force-pushed the fix-36837 branch from 626c193 to ff15d97 Compare November 18, 2023 16:05

bvanjoi commented Nov 18, 2023

View reviewed changes

tests/ui/imports/glob-conflict-cross-crate.rs Show resolved Hide resolved

bvanjoi commented Nov 18, 2023

View reviewed changes

tests/ui/imports/glob-conflict-cross-crate-2.rs Show resolved Hide resolved

tests/ui/imports/glob-conflict-cross-crate-3.rs Show resolved Hide resolved

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 18, 2023

This comment was marked as resolved.

Sign in to view

petrochenkov reviewed Dec 8, 2023

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 8, 2023

resolve: re-export ambiguity in extern crate as warning

45272bb

bvanjoi force-pushed the fix-36837 branch from 73c5062 to 45272bb Compare January 9, 2024 06:57

This was referenced Jan 9, 2024

fix: fix glob reolution issue thesimplekid/manage-relay-users#3

Open

fix: fix glob resolution issue polywrap/rust-wrap-client#259

Open

fix: fix glob resolution issue diesel-rs/diesel#3898

Open

petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 9, 2024

weiznich mentioned this pull request Jan 12, 2024

Fix most type-defs for #[auto_type] by cleaning up all the type defs diesel-rs/diesel#3783

Merged

petrochenkov reviewed Jan 17, 2024

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 17, 2024

petrochenkov mentioned this pull request Jan 21, 2024

add several resolution test cases #120195

Merged

JohnCSimon closed this Apr 14, 2024

rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Apr 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resolve: re-export ambiguity as warning #114682

resolve: re-export ambiguity as warning #114682

bvanjoi commented Aug 10, 2023 •

edited

bvanjoi commented Sep 12, 2023

petrochenkov commented Sep 13, 2023

petrochenkov commented Oct 7, 2023

JohnCSimon commented Nov 12, 2023

bvanjoi commented Nov 13, 2023

bvanjoi commented Nov 18, 2023

This comment was marked as resolved.

bvanjoi commented Jan 8, 2024

petrochenkov commented Jan 8, 2024

bvanjoi commented Jan 8, 2024

petrochenkov commented Jan 8, 2024

bvanjoi commented Jan 9, 2024

bvanjoi commented Jan 10, 2024

Ten0 commented Jan 12, 2024

bors commented Jan 13, 2024

petrochenkov commented Jan 17, 2024

petrochenkov commented Jan 17, 2024 •

edited

petrochenkov Jan 17, 2024

petrochenkov Jan 17, 2024

petrochenkov Jan 17, 2024

petrochenkov Jan 17, 2024

weiznich commented Jan 18, 2024

bvanjoi commented Jan 31, 2024 •

edited

bvanjoi commented Jan 31, 2024

weiznich commented Feb 2, 2024 •

edited

bvanjoi commented Feb 2, 2024

JohnCSimon commented Apr 14, 2024

	for child in &children {
	for child in &self.cstore().ambiguity_module_children_untracked(def_id, self.tcx.sess) {

	if ambiguity {
	if binding.is_ambiguity() && !binding.warn_ambiguity {

resolve: re-export ambiguity as warning #114682

resolve: re-export ambiguity as warning #114682

Conversation

bvanjoi commented Aug 10, 2023 • edited

bvanjoi commented Sep 12, 2023

petrochenkov commented Sep 13, 2023

petrochenkov commented Oct 7, 2023

JohnCSimon commented Nov 12, 2023

bvanjoi commented Nov 13, 2023

bvanjoi commented Nov 18, 2023

This comment was marked as resolved.

bvanjoi commented Jan 8, 2024

petrochenkov commented Jan 8, 2024

bvanjoi commented Jan 8, 2024

petrochenkov commented Jan 8, 2024

bvanjoi commented Jan 9, 2024

bvanjoi commented Jan 10, 2024

Ten0 commented Jan 12, 2024

bors commented Jan 13, 2024

petrochenkov commented Jan 17, 2024

petrochenkov commented Jan 17, 2024 • edited

petrochenkov Jan 17, 2024

Choose a reason for hiding this comment

petrochenkov Jan 17, 2024

Choose a reason for hiding this comment

petrochenkov Jan 17, 2024

Choose a reason for hiding this comment

petrochenkov Jan 17, 2024

Choose a reason for hiding this comment

weiznich commented Jan 18, 2024

bvanjoi commented Jan 31, 2024 • edited

bvanjoi commented Jan 31, 2024

weiznich commented Feb 2, 2024 • edited

bvanjoi commented Feb 2, 2024

JohnCSimon commented Apr 14, 2024

bvanjoi commented Aug 10, 2023 •

edited

petrochenkov commented Jan 17, 2024 •

edited

bvanjoi commented Jan 31, 2024 •

edited

weiznich commented Feb 2, 2024 •

edited