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

block ambiguous type conversion dotcalls in generics #22375

Merged
merged 1 commit into from
Aug 9, 2023

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Aug 3, 2023

fixes #22373

In normal code, when an ambiguous type symbol is called, a type conversion is not considered and routine overloading is done instead:

Nim/compiler/semexprs.nim

Lines 3113 to 3115 in b40da81

let ambig = c.isAmbiguous
if not (n[0].kind in {nkClosedSymChoice, nkOpenSymChoice, nkIdent} and ambig) and n.len == 2:
result = semConv(c, n, expectedType)

For the same situation in generic code with dotcalls, replace the ambiguous type symbol with a sym choice, which similarly ignores type conversions and performs normal routine overloading.

@Araq
Copy link
Member

Araq commented Aug 3, 2023

Shouldn't instead we always treat T(x) as a call and if T resolves to a type it's reinterpreted as a type conversion? Seems more consistent and easier to specify.

@metagn
Copy link
Collaborator Author

metagn commented Aug 4, 2023

You mean like

# a.nim
type T* = uint

# b.nim
template T*(x: int): float = float(x)

# c.nim
import a, b
# or
import b, a

doAssert T(123) is uint

?

This would require that we check all routine symbols for ambiguity with a type symbol, then check if such a type symbol is not ambiguous with another type symbol, and then pick the type symbol. This is more complex IMO than just making type symbols "weak" in calls and picking them only when it's clear we are dealing with a single type (the behavior in regular Nim and the behavior in generics with this PR).

@Araq
Copy link
Member

Araq commented Aug 6, 2023

In your example T(123) should resolve to the template as that is a better match than the type conversion. Alternatively maybe we can somehow detect it as ambiguous. But either way overload resolution should handle it.

@metagn
Copy link
Collaborator Author

metagn commented Aug 8, 2023

Putting this logic in overload resolution would be a hefty refactor. And honestly I don't think it's worth it, types and routines having the same name is a niche case, the logic is good enough IMO and we can focus on compatibility.

@Araq Araq merged commit 3aaef9e into nim-lang:devel Aug 9, 2023
16 checks passed
@Araq
Copy link
Member

Araq commented Aug 9, 2023

I don't agree, it's fundamentally the right thing to do instead of this rats nest of special cases. But it improves the situation so I merged it.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2023

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from 3aaef9e

Hint: mm: orc; opt: speed; options: -d:release
169269 lines; 9.806s; 611.645MiB peakmem

narimiran pushed a commit that referenced this pull request Aug 11, 2023
Araq pushed a commit that referenced this pull request Aug 25, 2024
…le types in symchoices (#23997)

fixes #23898, supersedes #23966 and #23990

Since #20631 ambiguous type symbols in templates are rejected outright,
now we generate a symchoice for type nodes if they're ambiguous, a
generalization of what was done in #22375. This is done for generics as
well. Symchoices also handle type symbols better now, ensuring their
type is a `typedesc` type; this probably isn't necessary for everything
to work but it makes the logic more robust.

Similar to #23989, we have to prepare for the fact that ambiguous type
symbols behave differently than normal type symbols and either error
normally or relegate to other routine symbols if the symbol is being
called. Generating a symchoice emulates this behavior, `semExpr` will
find the type symbol first, but since the symchoice has other symbols,
it will count as an ambiguous type symbol.

I know it seems spammy to carry around an ambiguity flag everywhere, but
in the future when we have something like #23104 we could just always
generate a symchoice, and the symchoice itself would carry the info of
whether the first symbol was ambiguous. But this could harm compiler
performance/memory use, it might be better to generate it only when we
have to, which in the case of type symbols is only when they're
ambiguous.
narimiran pushed a commit that referenced this pull request Sep 16, 2024
…le types in symchoices (#23997)

fixes #23898, supersedes #23966 and #23990

Since #20631 ambiguous type symbols in templates are rejected outright,
now we generate a symchoice for type nodes if they're ambiguous, a
generalization of what was done in #22375. This is done for generics as
well. Symchoices also handle type symbols better now, ensuring their
type is a `typedesc` type; this probably isn't necessary for everything
to work but it makes the logic more robust.

Similar to #23989, we have to prepare for the fact that ambiguous type
symbols behave differently than normal type symbols and either error
normally or relegate to other routine symbols if the symbol is being
called. Generating a symchoice emulates this behavior, `semExpr` will
find the type symbol first, but since the symchoice has other symbols,
it will count as an ambiguous type symbol.

I know it seems spammy to carry around an ambiguity flag everywhere, but
in the future when we have something like #23104 we could just always
generate a symchoice, and the symchoice itself would carry the info of
whether the first symbol was ambiguous. But this could harm compiler
performance/memory use, it might be better to generate it only when we
have to, which in the case of type symbols is only when they're
ambiguous.

(cherry picked from commit 09dcff7)
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.

Regression from 1.6 to 2.0 and devel with generics across modules
2 participants