[Sema] Always consider outer alternatives when resolving a decl ref.

[rxwei]

Currently, when a type member has the same base name as a global declaration, one cannot refer to the global member from a method context, and the current diagnostics force the user to use a fully qualified name. This is a long-standing issue with the use of top-level min and max in an extension, and is causing unnecessarily verbosity in math algorithms in extensions for ElementaryFunctions-conforming types.

More context:

1. Forum thread: ElementaryFunctions and ambiguity in extensions
2. SR-456: Confusing build error when calling 'max' function within 'extension Int'.
3. SR-1772: File-level function with the same name as instance function not picked up by compiler.

This PR makes UnresolvedDeclRefExpr resolution always consider alternative candidates from the outer context. The following code will no longer trigger any error or warning.

import Foundation // which exports a top-level `sin(_:)`.

extension Float {
    func foo() {
        // Refers to top-level `sin(_:)`, not static method `Float.sin(_:)`.
        _ = sin(self)
        _ = sin(_:) as (Float) -> Float
    }
}

extension Sequence {
    func foo() {
        // Refers to top-level `max(_:_:)`.
        _ = max(1, 2)
    }
}

[rxwei]

cc @stephentyrone @DougGregor @xedin @jrose-apple

If this change is the right direction, I'll go ahead and update tests. Thanks!

[airspeedswift]

@swift-ci please test source compatibility

[jrose-apple]

I really want this to go through full review, but the core team might disagree. It is definitely source-breaking, but it's possible no one will notice in practice.

Please make sure you test multiply-nested contexts, mixes of variables and functions, and wacky things like initializers as well.

[stephentyrone]

@jrose-apple agreed about test coverage. What are the issues that you think merit evolution review for this change?

Either way, we definitely also need a test to make sure this continues to work =)

[jrose-apple]

Here's the simplest source-breaking case I can think of:

func foo(_ x: Int) { print("top-level") }

struct X {
  func foo(_ x: Any) { print("method") }
  func test() { foo(1) }
}

X().test()

But even if everyone is universally in favor of this change (or at least neutral like me), I think it's a significant enough change to deserve attention on the forums, rather than just an entry in the changelog.

[stephentyrone]

@jrose-apple Testing Richard's branch on my machine, the behavior doesn't seem to be any different for that case; both master and Richard's branch compile without a warning and print method. For me, the main question is what the effect on compile time is going to be with this change.

[xedin]

Unfortunately proper fix for this problem is more involved than this to account for diagnostics and source compatibility:

• Remove Unresolved*Expr/DeclRefExpr expressions are cache lookup results in the constraint system.
• Adjust solver to re-fetch additional outer results when no solution could be formed from the local results; Possibly warn add suggest to add <Module>. prefix.
• CSApply would have to be adjusted to support direct transformation from UnresolvedDeclRefExpr to type-checked AST.

[xedin]

@stephentyrone The effect on the compile type in most part is the reason why this is done the way it is today. I think Huon measured that when he was working on the changes, it was two fold - performing lookup itself wasn't cheap, plus additional overloads created problems.

[jrose-apple]

Testing Richard's branch on my machine, the behavior doesn't seem to be any different for that case; both master and Richard's branch compile without a warning and print method. For me, the main question is what the effect on compile time is going to be with this change.

Ah, I misinterpreted the patch, then. I'm not sure I like it being a second-tier thing either…and that's why it's worth discussing.

[xedin]

@stephentyrone @jrose-apple I think that's because of the "favoring" hack for the simple candidate case, this example would not be source compatible:

func foo(_ x: Int) { print("top-level") }

struct X {
  func foo(_ x: Any) { print("Any") }
  func foo(_ x: String) { print("String") }

  func test() { foo(1) }
}

X().test()

[airspeedswift]

I really want this to go through full review, but the core team might disagree.

My personal (not official core team) view is that this is, in principle, a bug fix, as long as the implementation is correct and the source breakage is minimal and understandable.

[xedin]

@airspeedswift I agree and doing the way I have described is going to result in a straightforward rule with no breakage of the existing code because outer context overloads are going to be used as a fallback when no local solutions could be found.

[jrose-apple]

My opinion: Discussing these alternatives and their effect on source compatibility is important and should be written down, and the Swift Evolution Process provides an existing framework for that.

[rxwei]

Thanks for the feedback! I don't currently have the bandwidth to document or pitch this change. If someone can start a thread, that'd be great! I'll go ahead and implement the changes @xedin suggested some time this week.

[rxwei]

• Remove Unresolved*Expr/DeclRefExpr expressions are cache lookup results in the constraint system.

@xedin I'm not sure which lookup result cache you are referring to. Would you mind providing some specific pointers on this?

[xedin]

@rxwei Sure! We need to add one more cache to constraint system to hold overload choices per locator, that's currently a function of OverloadedDeclSetRefExpr and partially UnresolvedDotExpr. In pre-check phrase resolveDeclRefExpr mutates AST to introduce these expressions to record results of initial lookup performed on UnresolvedDeclRefExpr, but that happens too early and should be moved to the solver instead that way we can control what/when is something looked up.

Changes like that mean that we'd have to introduce a new constraint or extend use of member of constraint to operators and change the solver to account for the fact that disjunctions are not going to be available upfront. Also (and that's probably the worst part) one of the "optimizations" have to be moved from constraint generator into the solver beforehand.

[rxwei]

Thanks for the explanation @xedin! This is beyond my knowledge about Sema, and I don't think I'll be able to fix this in the short term. I'll leave the PR open for someone to pick up then.

[airspeedswift]

FYI, the core team discussed this and considers it a bug fix not in need of an evolution proposal. Though it sounds like it needs more work in the short term anyway.

[rxwei]

@xedin Is there a quick short-term solution without having to add new constraints or change operator type checking, something along the lines of the implementation in this PR? That way we can fix the bug sooner to solve some pressing issues: currently, the extensibility of the TensorFlow library is suffering from this bug.

[xedin]

@rxwei I don't see a way to maintain source compatibility and do something more localized without making this even worse... I originally started looking at this in context of diagnosing name shadowing e.g. https://bugs.swift.org/browse/SR-9096

[CodaFi]

@rxwei Are you still pursuing this?

[rxwei]

Nope. Would you like to take over?

[shahmishal]

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

• How to change the base branch: (Link)
• More detail about the branch update: (Link)

[rxwei]

Closing since I'm no longer pursuing this.