adapt generic default parameters to recent generics changes #24065
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
metagn
changed the title
|
Thanks for your hard work on this PR! Hint: mm: orc; opt: speed; options: -d:release |
metagn
added a commit
to metagn/Nim
that referenced
this pull request
Sep 6, 2024
fixes CI, refs nim-lang#24066, refs nim-lang#24065
Araq
pushed a commit
that referenced
this pull request
Sep 8, 2024
fixes CI, refs #24066, refs #24065 The combination of #24065 and #24066 caused a CI failure where a `when` branch that was never compiled gave an undeclared identifier error. This is because the `when` branch was being semchecked with `semGenericStmt` without `withinMixin`, which is the flag `semgnrc` normally gives to `when` branch bodies. To fix this, just pass the whole `when` stmt to `semGenericStmt` rather than the individual blocks. The alternative would be to just replace the calls to `semGenericStmt` with a new proc that does the same thing, just with the flags `{withinMixin}`.
Araq
pushed a commit
that referenced
this pull request
Sep 11, 2024
fixes #23506 #24065 broke compilation of template parameter default values that depended on other template parameters. But this was never implemented anyway, actually attempting to use those default values breaks the compiler as in #23506. So these are now implemented as well as fixing the regression. First, if a default value expression uses any unresolved arguments (generic or normal template parameters) in a template header, we leave it untyped, instead of applying the generic typechecking (fixing the regression). Then, just before the body of the template is about to be explored, the default value expressions are handled in the same manner as the body as well. This captures symbols including the parameters, so the expression is checked again if it contains a parameter symbol, and marked with `nfDefaultRefsParam` if it does (as an optimization to not check it later). Then when the template is being evaluated, when substituting a parameter, if we try to substitute with a node marked `nfDefaultRefsParam`, we also evaluate it as we would the template body instead of passing it as just a copy (the reason why it never worked before). This way we save time if a default value doesn't refer to another parameter and could just be copied regardless.
Araq
pushed a commit
that referenced
this pull request
Sep 27, 2024
Araq
pushed a commit
that referenced
this pull request
Nov 6, 2024
fixes issue described in https://forum.nim-lang.org/t/12579 In #24065 explicit generic parameter matching was made to fail matches on arguments with unresolved types in generic contexts (the sigmatch diff, following #24010), similar to what is done for regular calls since #22029. However unlike regular calls, a failed match in a generic context for a standalone explicit generic instantiation did not convert the expression into one with `tyFromExpr` type, which means it would error immediately given any unresolved parameter. This is now done to fix the issue. For explicit generic instantiations on single non-overloaded symbols, a successful match is still instantiated. For multiple overloads (i.e. symchoice), if any of the overloads fail the match, the entire expression is considered untyped and any instantiations are not used, so as to not void overloads that would match later. This means even symchoices without unresolved arguments aren't instantiated, which may be too restrictive, but it could also be too lenient and we might need to make symchoice instantiations always untyped. The behavior for symchoice is not sound anyway given it causes #9997 so this is something to consider for a redesign. Diff follows #24276.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #16700, fixes #20916, refs #24010
Fixes the instantiation issues for proc param default values encountered in #24010 by:
inGenericContextfor fix calls in generic bodies, delay typecheck when no overloads match #22029 and followups to apply (the bigger change in semtypes),inGenericContext(sigmatch change),prepareNoderather than an insufficient manual method (the bigger change in seminst).This had an important side effect of references to other parameters not working since they would be resolved as a symbol belonging to the uninstantiated original generic proc rather than the later instantiated proc. There is a more radical way to fix this which is generating ident nodes with
tyFromExprin specifically this context, but instead we just count them as belonging to the same proc inhoistParamsUsedInDefault.Other minor bugfixes:
tyFromExpr.evalConstExprmore than once, but the VM doesn't mark the object fields asnfSkipFieldCheckingwhich gives a "cannot access field" error. So the VM now marks object fields it generates asnfSkipFieldChecking. Not sure if this affects VM performance, don't see why it would.whenin objects #24042 didn't cover the case where all conditions are constantly false correctly, this is fixed with a minor change. This isn't needed for this PR now but I encountered it after forgetting todec c.inGenericContext.