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.

Sign up for GitHub

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

Jump to bottom

fix #15836 proc arg return type auto unexpectly match proc with concr… #21065

Merged
merged 4 commits into from
Dec 12, 2022
Merged

fix #15836 proc arg return type auto unexpectly match proc with concr… #21065

merged 4 commits into from
Dec 12, 2022

Conversation

bung87
Copy link
Collaborator

@bung87 bung87 commented Dec 10, 2022 •
edited

…ete type
fix #15836
fix #16244
fix #12869

refs: #16906 now call converstation error
refs: #16975 now compiler/ccgtypes.nim(192, 17) false mapType: tyAnything [AssertionDefect]
refs: #7435 generic with auto
refs: #18866 generic with auto

so the issue change to a complex one instead of a simple one, the test case in tests/types/t15836_2.nim is taken from https://github.com/mratsim/Arraymancer/blob/master/tests/nn_primitives/test_nnp_convolution.nim which is disabled for now.
discussed in mratsim/Arraymancer#580

@bung87 bung87 mentioned this pull request Dec 11, 2022
Closed
@Araq Araq merged commit 5917c2d into nim-lang:devel Dec 12, 2022
@github-actions
Copy link
Contributor

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

Hint: mm: orc; opt: speed; options: -d:release
165632 lines; 11.892s; 612.176MiB peakmem

survivorm pushed a commit to survivorm/Nim that referenced this pull request Feb 28, 2023
@bung87 @survivorm
fix nim-lang#15836 proc arg return type auto unexpectly match proc wi…
b52d41f 
…th concr… (nim-lang#21065)

* fix nim-lang#15836 proc arg return type auto unexpectly match proc with concrete type

* fix nim-lang#16244

* add test case for nim-lang#12869
capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
@bung87 @capocasa
fix nim-lang#15836 proc arg return type auto unexpectly match proc wi…
ceac469 
…th concr… (nim-lang#21065)

* fix nim-lang#15836 proc arg return type auto unexpectly match proc with concrete type

* fix nim-lang#16244

* add test case for nim-lang#12869
narimiran pushed a commit that referenced this pull request Apr 26, 2023
@bung87 @narimiran
fix #15836 proc arg return type auto unexpectly match proc with concr… (
4ea0ce9 
#21065)

* fix #15836 proc arg return type auto unexpectly match proc with concrete type

* fix #16244

* add test case for #12869

(cherry picked from commit 5917c2d)
bung87 added a commit to bung87/Nim that referenced this pull request Jul 29, 2023
@bung87
fix nim-lang#15836 proc arg return type auto unexpectly match proc wi…
9221e24 
…th concr… (nim-lang#21065)

* fix nim-lang#15836 proc arg return type auto unexpectly match proc with concrete type

* fix nim-lang#16244

* add test case for nim-lang#12869
@metagn metagn mentioned this pull request Jan 14, 2024
Merged
Araq pushed a commit that referenced this pull request Jan 17, 2024
@metagn
don't use previous bindings of auto for routine return types (#23207)
f46f26e 
fixes #23200, fixes #18866

#21065 made it so `auto` proc return types remained as `tyAnything` and
not turned to `tyUntyped`. This had the side effect that anything
previously bound to `tyAnything` in the proc type match was then bound
to the proc return type, which is wrong since we don't know the proc
return type even if we know the expected parameter types (`tyUntyped`
also [does not care about its previous bindings in
`typeRel`](https://github.com/nim-lang/Nim/blob/ab4278d2179639f19967431a7aa1be858046f7a7/compiler/sigmatch.nim#L1059-L1061)
maybe for this reason).

Now we mark `tyAnything` return types for routines as `tfRetType` [as
done for other meta return
types](https://github.com/nim-lang/Nim/blob/18b5fb256d4647efa6a64df451d37129d36e96f3/compiler/semtypes.nim#L1451),
and ignore bindings to `tyAnything` + `tfRetType` types in `semtypinst`.
On top of this, we reset the type relation in `paramTypesMatch` only
after creating the instantiation (instead of trusting
`isInferred`/`isInferredConvertible` before creating the instantiation),
using the same mechanism that `isBothMetaConvertible` uses.

This fixes the issues as well as making the disabled t15386_2 test
introduced in #21065 work. As seen in the changes for the other tests,
the error messages give an obscure `proc (a: GenericParam): auto` now,
but it does give the correct error that the overload doesn't match
instead of matching the overload pre-emptively and expecting a specific
return type.

tsugar had to be changed due to #16906, which is the problem where
`void` is not inferred in the case where `result` was never touched.
narimiran pushed a commit that referenced this pull request Apr 19, 2024
@metagn @narimiran
don't use previous bindings of auto for routine return types (#23207)
ff6fa3a 
fixes #23200, fixes #18866

#21065 made it so `auto` proc return types remained as `tyAnything` and
not turned to `tyUntyped`. This had the side effect that anything
previously bound to `tyAnything` in the proc type match was then bound
to the proc return type, which is wrong since we don't know the proc
return type even if we know the expected parameter types (`tyUntyped`
also [does not care about its previous bindings in
`typeRel`](https://github.com/nim-lang/Nim/blob/ab4278d2179639f19967431a7aa1be858046f7a7/compiler/sigmatch.nim#L1059-L1061)
maybe for this reason).

Now we mark `tyAnything` return types for routines as `tfRetType` [as
done for other meta return
types](https://github.com/nim-lang/Nim/blob/18b5fb256d4647efa6a64df451d37129d36e96f3/compiler/semtypes.nim#L1451),
and ignore bindings to `tyAnything` + `tfRetType` types in `semtypinst`.
On top of this, we reset the type relation in `paramTypesMatch` only
after creating the instantiation (instead of trusting
`isInferred`/`isInferredConvertible` before creating the instantiation),
using the same mechanism that `isBothMetaConvertible` uses.

This fixes the issues as well as making the disabled t15386_2 test
introduced in #21065 work. As seen in the changes for the other tests,
the error messages give an obscure `proc (a: GenericParam): auto` now,
but it does give the correct error that the overload doesn't match
instead of matching the overload pre-emptively and expecting a specific
return type.

tsugar had to be changed due to #16906, which is the problem where
`void` is not inferred in the case where `result` was never touched.

(cherry picked from commit f46f26e)
narimiran pushed a commit that referenced this pull request Apr 27, 2024
@metagn @narimiran
don't use previous bindings of auto for routine return types (#23207)
d23b78a 
fixes #23200, fixes #18866

not turned to `tyUntyped`. This had the side effect that anything
previously bound to `tyAnything` in the proc type match was then bound
to the proc return type, which is wrong since we don't know the proc
return type even if we know the expected parameter types (`tyUntyped`
also [does not care about its previous bindings in
`typeRel`](https://github.com/nim-lang/Nim/blob/ab4278d2179639f19967431a7aa1be858046f7a7/compiler/sigmatch.nim#L1059-L1061)
maybe for this reason).

Now we mark `tyAnything` return types for routines as `tfRetType` [as
done for other meta return
types](https://github.com/nim-lang/Nim/blob/18b5fb256d4647efa6a64df451d37129d36e96f3/compiler/semtypes.nim#L1451),
and ignore bindings to `tyAnything` + `tfRetType` types in `semtypinst`.
On top of this, we reset the type relation in `paramTypesMatch` only
after creating the instantiation (instead of trusting
`isInferred`/`isInferredConvertible` before creating the instantiation),
using the same mechanism that `isBothMetaConvertible` uses.

This fixes the issues as well as making the disabled t15386_2 test
introduced in #21065 work. As seen in the changes for the other tests,
the error messages give an obscure `proc (a: GenericParam): auto` now,
but it does give the correct error that the overload doesn't match
instead of matching the overload pre-emptively and expecting a specific
return type.

tsugar had to be changed due to #16906, which is the problem where
`void` is not inferred in the case where `result` was never touched.

(cherry picked from commit f46f26e)
narimiran pushed a commit that referenced this pull request Apr 27, 2024
@metagn @narimiran
don't use previous bindings of auto for routine return types (#23207)
4e5680b 
fixes #23200, fixes #18866

not turned to `tyUntyped`. This had the side effect that anything
previously bound to `tyAnything` in the proc type match was then bound
to the proc return type, which is wrong since we don't know the proc
return type even if we know the expected parameter types (`tyUntyped`
also [does not care about its previous bindings in
`typeRel`](https://github.com/nim-lang/Nim/blob/ab4278d2179639f19967431a7aa1be858046f7a7/compiler/sigmatch.nim#L1059-L1061)
maybe for this reason).

Now we mark `tyAnything` return types for routines as `tfRetType` [as
done for other meta return
types](https://github.com/nim-lang/Nim/blob/18b5fb256d4647efa6a64df451d37129d36e96f3/compiler/semtypes.nim#L1451),
and ignore bindings to `tyAnything` + `tfRetType` types in `semtypinst`.
On top of this, we reset the type relation in `paramTypesMatch` only
after creating the instantiation (instead of trusting
`isInferred`/`isInferredConvertible` before creating the instantiation),
using the same mechanism that `isBothMetaConvertible` uses.

This fixes the issues as well as making the disabled t15386_2 test
introduced in #21065 work. As seen in the changes for the other tests,
the error messages give an obscure `proc (a: GenericParam): auto` now,
but it does give the correct error that the overload doesn't match
instead of matching the overload pre-emptively and expecting a specific
return type.

tsugar had to be changed due to #16906, which is the problem where
`void` is not inferred in the case where `result` was never touched.

(cherry picked from commit f46f26e)
narimiran pushed a commit that referenced this pull request Apr 27, 2024
@metagn @narimiran
don't use previous bindings of auto for routine return types (#23207)
b302b31 
fixes #23200, fixes #18866

not turned to `tyUntyped`. This had the side effect that anything
previously bound to `tyAnything` in the proc type match was then bound
to the proc return type, which is wrong since we don't know the proc
return type even if we know the expected parameter types (`tyUntyped`
also [does not care about its previous bindings in
`typeRel`](https://github.com/nim-lang/Nim/blob/ab4278d2179639f19967431a7aa1be858046f7a7/compiler/sigmatch.nim#L1059-L1061)
maybe for this reason).

Now we mark `tyAnything` return types for routines as `tfRetType` [as
done for other meta return
types](https://github.com/nim-lang/Nim/blob/18b5fb256d4647efa6a64df451d37129d36e96f3/compiler/semtypes.nim#L1451),
and ignore bindings to `tyAnything` + `tfRetType` types in `semtypinst`.
On top of this, we reset the type relation in `paramTypesMatch` only
after creating the instantiation (instead of trusting
`isInferred`/`isInferredConvertible` before creating the instantiation),
using the same mechanism that `isBothMetaConvertible` uses.

This fixes the issues as well as making the disabled t15386_2 test
introduced in #21065 work. As seen in the changes for the other tests,
the error messages give an obscure `proc (a: GenericParam): auto` now,
but it does give the correct error that the overload doesn't match
instead of matching the overload pre-emptively and expecting a specific
return type.

tsugar had to be changed due to #16906, which is the problem where
`void` is not inferred in the case where `result` was never touched.

(cherry picked from commit f46f26e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@bung87 @Araq