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

remove allowMetaTypes for inferStaticParam + properly annotate unresolved range #24086

Open
wants to merge 3 commits into
base: devel
Choose a base branch
from

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Sep 8, 2024

fixes #19923

The code in #19923 already compiles since #24005, but when actually trying to pass an argument to it, sigmatch fails to infer the array type (but does infer the range type). There are 2 reasons:

  1. The expression self.S * 8 is a tyFromExpr which for some reason results in the range type not being marked tfUnresolved, and so array types just test for range span equality. This is because makeRangeWithStaticExpr only sets tfUnresolved if the given expression type isn't nil and has a nil node which is never true for tyFromExpr. This is fixed by just manually setting tfUnresolved. (also for when the node hasUnresolvedArgs but this isn't necessary here)
  2. The compiler can't resolve self.S at overload resolution time since inferStaticParam enables allowMetaTypes, which results in tyFromExprs never getting instantiated. To fix this we turn off allowMetaTypes.

Turning off allowMetaTypes here has several consequences:

  • The types undergo instCopyType which changes their ID, so to give their bindings properly we have to get the original static type ID, which is done here by doing .sym.typ on the static type, since the sym is invariant between type copies. Thankfully only inferStaticParam gives a binding here.
  • Inability to instantiate directly gives an error instead of waiting for overloads to finish. This is both due to the nature of semtypinst and the call to failureToInferStaticParam. In practice this isn't encountered often (mostly because it wasn't possible before) but it can cause unrelated overloads to be inaccessible if an overload requires explicit generic arguments. If this becomes a problem we can refactor semtypinst and failureToInferStaticParam to a version that just doesn't globally error on these and passes the information to the mismatch errors. Edit: Done in consider instantiation errors in overload resolution as type mismatch #24098

@metagn metagn changed the title test removing allowMetaTypes for inferStaticParam remove allowMetaTypes for inferStaticParam + properly annotate unresolved range Sep 8, 2024
@arnetheduck
Copy link
Contributor

Inability to instantiate directly gives an error instead of waiting for overloads to finish

shouldn't this just cause it to not be considered? sounds similar to C++ sfinae, which is a nice feature in general..

@metagn
Copy link
Collaborator Author

metagn commented Sep 12, 2024

shouldn't this just cause it to not be considered?

Yep, I started implementing it in #24098, which this PR should probably follow. Good point about SFINAE, I was debating some refactors (not doing inc c.inGenericContext when matching tyFromExpr as mentioned in #24095, restricting the "calls with no matching overloads are untyped" to just matching unresolved types) that would have made the compiler error on stuff like:

template foo(T: type int): untyped = float

proc bar[T](x: T) = echo "a"
proc bar[T](x: foo(T)) = echo "b"

bar(12.3) # a
bar[int](12.3) # b
bar[float](12.3) # error

Should probably write a spec about this stuff after it's all figured out

Edit: Actually this still errors because explicit instantiations can't handle it, but those refactors also would have broken it

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.

Proc argument type derived from expression with generic type argument causes segfault in compiler
2 participants