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

overload lookup fixes #519

Merged
merged 5 commits into from
Feb 12, 2025
Merged

overload lookup fixes #519

merged 5 commits into from
Feb 12, 2025

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Feb 12, 2025

Overload lookup is changed to match the old compiler: Callee symbols no longer use FindAll, they use a new lookup mode FindOverloads, which only considers overloads of unambiguous symbols if they are overloadable. InnerMost always returns a symbol if it is unambiguous, even if it is overloadable. (before InnerMost behaved like FindOverloads)

This means callee symbols prefer local unambiguous symbols to outer overloads. To deal with this when the local symbols are not procs (i.e. let len = arr.len), in the case where the callee symbol is not callable, all overloads are considered.

For this to work, symchoice callees now ignore non-callable symbols and give an error if none of the choices were callable (the previous error would have been "undeclared identifier").

This also means when local unoverloadable symbols do have proc types, they are matched directly and outer overloads are not considered, which is also the behavior of the old compiler (local types are the same):

proc foo(x: int) = discard

proc main =
  let foo = proc (x: string) = discard
  foo(123) # error

But it's open how this should behave. nim-lang/Nim#24357

Also when where no branch is evaluated is fixed to produce a void type instead of auto.

@metagn metagn marked this pull request as ready for review February 12, 2025 12:35
@Araq
Copy link
Member

Araq commented Feb 12, 2025

Callee symbols no longer use FindAll, they use a new lookup mode FindOverloads, which only considers overloads of unambiguous symbols if they are overloadable.

Sounds hard to understand to me. In Nim 2 this whole "symbol choice is open when overloaded" was a mild desaster where proc foo(x: int); proc foo(x: float) is different from proc foo(x: int|float). For the --compat switch this way is ok but for Nim 3 we should have easier to teach rules.

What bug does this fix and how does it fix it?

@metagn
Copy link
Collaborator Author

metagn commented Feb 12, 2025

The bug in tscopedtype and the bug that was in tmimalloc, a local type with the same name as an imported type (or any imported symbol) would always be considered ambiguous otherwise. It would also cause variables/constants with proc types to participate in overloading, which is mostly an incompatibility, but means a more specific overload getting added to scope could change the meaning of a call to a local symbol which is a kind of behavior people have complained about.

The complexity is from the difference between things that participate in overloading and things that can be called, overloads disambiguate using scope as a last resort (not implemented) but types and variables are supposed to consider scope immediately. There is just an additional rule that if something that resolves immediately cannot be called (as in it does not make sense to call it), overloads are also attempted instead of immediately erroring with "why are you calling this integer". It's a consequence of the call syntax itself being overloaded instead of having 2 different syntaxes, one for calling overloads (foo(123)) and one for calling concrete values (Foo.(123) etc). It's like we're adding "match rules" to this overloading.

About FindOverloads specifically: it does not really represent an additional behavior of the language, it would be the same if we did InnerMost instead and called FindAll if it returned an overloadable symbol. It is just probably faster than merging it with the proc type check for variables which does the same thing.

@Araq Araq merged commit 4de49db into nim-lang:master Feb 12, 2025
3 checks passed
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.

2 participants