In Context::LookupUnqualifiedName(), prefer lexical result over non lexical result

[bricknerb]

No description provided.

[jonmeow]

This feels like a behavior change, but I'm having trouble coming up with a test case, so @zygoloid who put this together.

[jonmeow]

Note, it may be correct even if it's a behavior change -- but I think it changes behavior when there's a lexical result and later non-lexical scopes. The current code would look through the remaining non-lexical scopes first (starting with first_non_lexical_scope), whereas the change would use the lexical result.

[zygoloid]

This would do the wrong thing for a case like:

fn F();
class C {
  fn F() -> i32;
  fn G() -> i32;
}
fn C.G() -> i32 {
  return F();
}

Here, lexical lookup for F finds package.F, but the desired unqualified lookup result is instead C.F.

If we don't already have a testcase like this, we should add one.

[bricknerb]

This would do the wrong thing for a case like:

fn F();
class C {
  fn F() -> i32;
  fn G() -> i32;
}
fn C.G() -> i32 {
  return F();
}

Here, lexical lookup for F finds package.F, but the desired unqualified lookup result is instead C.F.

If we don't already have a testcase like this, we should add one.

I've tried to reproduce this and failed so far (can't see a difference in behavior).
I'll try to understand better when a lexical lookup would trigger, since the following code doesn't trigger a different behavior in Check.

fn F() {}

namespace N;
fn N.F() -> i32 { return 1; }
fn N.G() -> i32 { return F(); }

class C {
  fn F() -> i32 { return 2; }
  fn G() -> i32;
}
fn C.G() -> i32 {
  return F();
}

fn Main() {
  F();

  var namespace_func_result: i32;
  namespace_func_result = N.G();

  var class_func_result: i32;
  var class_instance: C;
  class_func_result = class_instance.G();
}

[zygoloid]

I've tried to reproduce this and failed so far (can't see a difference in behavior).

Oh, right: the outer name will need to come from a lexical scope for us to see a difference here. (Package scope is a non-lexical scope, so the example I mentioned and the namespace example won't be affected by this.) You would see a difference if a lexical scope encloses a non-lexical scope. Because we don't support local classes yet, I think that means the only way this is observable right now is if a non-lexical name shadows a generic parameter:

class Class(F:! type) {
  class Inner {
    fn F() -> i32 { return 0; }
    fn G() -> i32;
  }
}

fn Class(F:! type).Inner.G() -> i32 { return F(); }

... which compiles prior to this change and is rejected afterwards.

[bricknerb]

I've tried to reproduce this and failed so far (can't see a difference in behavior).

Oh, right: the outer name will need to come from a lexical scope for us to see a difference here. (Package scope is a non-lexical scope, so the example I mentioned and the namespace example won't be affected by this.) You would see a difference if a lexical scope encloses a non-lexical scope. Because we don't support local classes yet, I think that means the only way this is observable right now is if a non-lexical name shadows a generic parameter:

class Class(F:! type) {
  class Inner {
    fn F() -> i32 { return 0; }
    fn G() -> i32;
  }
}

fn Class(F:! type).Inner.G() -> i32 { return F(); }

... which compiles prior to this change and is rejected afterwards.

Thanks Richard!
This makes total sense.
I've copy-pasted your sample code into #4591.