fix generic instantiations of doArgParse
#233
Merged
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
`doArgParse` takes 2 generic parameters, but in `argParse` for each number type, only 1 is given, which compiles due to bugs in Nim. To safeguard against the Nim compiler, the missing generic parameter is now provided.
|
Looks good to me. Thanks! It's pretty easy to forget a parameter. Hopefully newer Nim versions will at least warn? |
The PR to Nim I'm working on, nim-lang/Nim#24010, makes them error, which is how I found this. I'm considering the idea of allowing something like |
Araq
pushed a commit
to nim-lang/Nim
that referenced
this pull request
Sep 2, 2024
fixes #16376 The way the compiler handled generic proc instantiations in calls (like `foo[int](...)`) up to this point was to instantiate `foo[int]`, create a symbol for the instantiated proc (or a symchoice for multiple procs excluding ones with mismatching generic param counts), then perform overload resolution on this symbol/symchoice. The exception to this was when the called symbol was already a symchoice node, in which case it wasn't instantiated and overloading was called directly ([these lines](https://github.com/nim-lang/Nim/blob/b7b1313d21deb687adab2b4a162e716ba561a26b/compiler/semexprs.nim#L3366-L3371)). This has several problems: * Templates and macros can't create instantiated symbols, so they couldn't participate in overloaded explicit generic instantiations, causing the issue #16376. * Every single proc that can be instantiated with the given generic params is fully instantiated including the body. #9997 is about this but isn't fixed here since the instantiation isn't in a call. The way overload resolution handles explicit instantiations by itself is also buggy: * It doesn't check constraints. * It allows only partially providing the generic parameters, which makes sense for implicit generics, but can cause ambiguity in overloading. Here is how this PR deals with these problems: * Overload resolution now always handles explicit generic instantiations in calls, in `initCandidate`, as long as the symbol resolves to a routine symbol. * Overload resolution now checks the generic params for constraints and correct parameter count (ignoring implicit params). If these don't match, the entire overload is considered as not matching and not instantiated. * Special error messages are added for mismatching/missing/extra generic params. This is almost all of the diff in `semcall`. * Procs with matching generic parameters now instantiate only the type of the signature in overload resolution, not the proc itself, which also works for templates and macros. Unfortunately we can't entirely remove instantiations because overload resolution can't handle some cases with uninstantiated types even though it's resolved in the binding (see the last 2 blocks in `texplicitgenerics`). There are also some instantiation issues with default params that #24005 didn't fix but I didn't want this to become the 3rd huge generics PR in a row so I didn't dive too deep into trying to fix them. There is still a minor instantiation fix in `semtypinst` though for subscripts in calls. Additional changes: * Overloading of `[]` wasn't documented properly, it somewhat is now because we need to mention the limitation that it can't be done for generic procs/types. * Tests can now enable the new type mismatch errors with just `-d:testsConciseTypeMismatch` in the command. Package PRs: - using fork for now: [combparser](PMunch/combparser#7) (partial generic instantiation) - merged: [cligen](c-blake/cligen#233) (partial generic instantiation but non-overloaded + template) - merged: [neo](andreaferretti/neo#56) (trying to instantiate template with no generic param)
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.
doArgParsetakes 2 generic parameters, but inargParsefor each number type, only 1 is given, which compiles due to bugs in Nim. To safeguard against the Nim compiler, the missing generic parameter is now provided.