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

don't use previous bindings of auto for routine return types #23207

Merged
merged 9 commits into from
Jan 17, 2024

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Jan 14, 2024

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 maybe for this reason).

Now we mark tyAnything return types for routines as tfRetType as done for other meta return types, 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.

@metagn metagn changed the title don't use binding of auto for inferred return type don't use binding of auto for inferred return type [backport:1.6] Jan 14, 2024
@metagn metagn changed the title don't use binding of auto for inferred return type [backport:1.6] don't use binding of auto for inferred return type Jan 15, 2024
@metagn metagn changed the title don't use binding of auto for inferred return type don't use binding of auto for return type Jan 16, 2024
@metagn metagn changed the title don't use binding of auto for return type don't use previous bindings of auto for routine return types Jan 16, 2024
@metagn
Copy link
Collaborator Author

metagn commented Jan 16, 2024

Should be cleaner now. Further improvements would be to pass f to semInferredLambda or generateInstance as the expected type of the body for better type inference.

CI failure is unrelated

@Araq Araq merged commit f46f26e into nim-lang:devel Jan 17, 2024
17 of 19 checks passed
Copy link
Contributor

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

Hint: mm: orc; opt: speed; options: -d:release
177746 lines; 7.551s; 768.082MiB peakmem

narimiran pushed a commit that referenced this pull request Apr 19, 2024
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
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
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
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
Labels
None yet
Projects
None yet
2 participants