Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include top-level symbols from same file in outer ambiguity error #17033

Merged
merged 1 commit into from
May 1, 2023

Conversation

lrytz
Copy link
Member

@lrytz lrytz commented Mar 2, 2023

When checking if an inherited definition is ambiguous with an outer definition, include top-level outer definitions defined in the same compilation unit.

Follow-up for #8622.

@lrytz lrytz force-pushed the ambiguous-package branch 3 times, most recently from f60ed45 to 3fcb75c Compare March 3, 2023 11:00
@lrytz lrytz requested a review from odersky March 3, 2023 14:20
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a it worried about the added complexity (both runtime and conceptual) this introduces. In the original PR there was this spec:

It is an error if an identifier x is available as an inherited member
in an inner scope and the same name x is defined in an outer scope, unless

  • the inherited member (has an overloaded alternative that) coincides with
    (an overloaded alternative of) the definition x
  • x is global, i.e. a member of a package.

I assume the last clause would have to be changed to this:

  • x is global, i.e. a member of a package, and it is defined not in the same source as the inner scope.

I am not sure we need this additional complication however. Have we observed cases in the wild where this is a problem that should be diagnosed?

@odersky odersky assigned lrytz and unassigned odersky Mar 7, 2023
@lrytz
Copy link
Member Author

lrytz commented Mar 7, 2023

I did come across such a case when deploying the 2.12 backport of this change at our customer, that's how I noticed.

The following is not ambiguous:

File 1

trait T { class X }

File 2

package p {
  class X
  class C extends T {
    def test = new X // resolves to this.X
  }
}

Changing package p to object p makes the X reference ambiguous, which I find surprising.

@odersky
Copy link
Contributor

odersky commented Mar 7, 2023

OK, I think we can change the spec as follows (and it would be good to include it as a doc comment):

It is an error if an identifier x is available as an inherited member in an inner scope
and the same name x is defined in an outer scope in the same source file, unless
the inherited member (has an overloaded alternative that) coincides with
(an overloaded alternative of) the definition x.

Then we can argue that it's a simplification, spec-wise.

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM

compiler/src/dotty/tools/dotc/typer/Typer.scala Outdated Show resolved Hide resolved
When checking if an inherited definition is ambiguous with an
outer definition, include top-level outer definitions defined
in the same compilation unit.
@odersky
Copy link
Contributor

odersky commented May 8, 2023

It seems this change caused a lot of regressions in the open community build. See #17433. We should probably revert it.

sjrd added a commit that referenced this pull request Jun 12, 2023
@odersky odersky added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Jul 10, 2023
@odersky
Copy link
Contributor

odersky commented Jul 10, 2023

If this is the error that regressed in 3.3.1 and succeeds in nightly it should be backported.

@lrytz
Copy link
Member Author

lrytz commented Jul 10, 2023

This PR was reverted in main (#17438), but it seems that revert is not in the 3.3.1 branch (https://github.com/lampepfl/dotty/blob/release-3.3.1/compiler/src/dotty/tools/dotc/typer/Typer.scala#L437).

Follow-up PR is here: #17439 (comment)

@Kordyjan Kordyjan removed the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Jul 11, 2023
Kordyjan added a commit that referenced this pull request Jul 11, 2023
…mbiguity error"" (#18182)

Backports #17438
Reverts #17033 that slipped through the cracks and got into 3.3.1,
causing a regression.
@Kordyjan Kordyjan added this to the 3.3.1 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants