Fix RLS issues, fix crash on invalid result type for @splat, refactor some bits of generic instantiations
#16604
Conversation
This file contains hidden or 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
mlugg
force-pushed
the
result-type-shenanigans
branch
from
5b08d30 to
59b7cc2
Compare
Member
Author
|
oh dear god the scope keeps expanding. pray for me |
mlugg
force-pushed
the
result-type-shenanigans
branch
3 times, most recently
from
ca29b6c to
fa73b40
Compare
mlugg
force-pushed
the
result-type-shenanigans
branch
from
fa73b40 to
5d778f8
Compare
mlugg
force-pushed
the
result-type-shenanigans
branch
3 times, most recently
from
eebe058 to
3ded752
Compare
This was referenced Aug 8, 2023
mlugg
changed the title
@splat@splat, refactor some bits of generic instantiations
Well, this was a journey! The original issue I was trying to fix is covered by the new behavior test in array.zig: in essence, `ty` and `coerced_ty` result locations were not correctly propagated. While fixing this, I noticed a similar bug in struct inits: the type was propagated to *fields* fine, but the actual struct init was unnecessarily anonymous, which could lead to unnecessary copies. Note that the behavior test added in struct.zig was already passing - the bug here didn't change any easy-to-test behavior - but I figured I'd add it anyway. This is a little harder than it seems, because the result type may not itself be an array/struct type: it could be an optional / error union wrapper. A new ZIR instruction is introduced to unwrap these. This is also made a little tricky by the fact that it's possible for result types to be unknown at the time of semantic analysis (due to `anytype` parameters), leading to generic poison. In these cases, we must essentially downgrade to an anonymous initialization. Fixing these issues exposed *another* bug, related to type resolution in Sema. That issue is now tracked by ziglang#16603. As a temporary workaround for this bug, a few result locations for builtin function operands have been disabled in AstGen. This is technically a breaking change, but it's very minor: I doubt it'll cause any breakage in the wild.
This introduces a new ZIR instruction, `vec_elem_type`. Co-Authored-By: Ali Chraghi <alichraghi@proton.me> Resolves: ziglang#16567
mlugg
force-pushed
the
result-type-shenanigans
branch
from
d137f9e to
9ff80a9
Compare
jacobly0
reviewed
Aug 9, 2023
andrewrk
requested changes
Aug 10, 2023
Member
andrewrk
left a comment
andrewrk
left a comment
There was a problem hiding this comment.
I'm only partially done reviewing this (made it to the 3rd commit) but I'm going to bed so I'll leave you what I have so far:
NOTE ON FUTURE WORK: In the future, we should work to reinstate the old generic instantiation caching logic, where the...
Not sure if I agree with this. Let's discuss. But I don't think that discussion needs to be in the commit message, or block this PR.
| elem_type, | ||
| /// Given a vector type, returns its element type. | ||
| /// Uses the `un_node` field. | ||
| vector_elem_type, |
Member
There was a problem hiding this comment.
There is already elem_type above; why must there also be vector_elem_type?
Member
Author
There was a problem hiding this comment.
Since this is for RLS, it's important for the instruction to be quite precise for good error messages. This isn't one of those instructions where the type is already validated: this comes before the splat, so is doing the validation. For instance, if you try to do @as(*@Vector(2, u32), @splat(123)), you want the error to be that a vector type was expected for the @splat, but if we use a general elem_type, we'd instead get expected type '@Vector(2, u32)', found 'comptime_int' on the operand. (Note that the error could seem even weirder if vectors aren't involved at all!)
Member
Author
My bad, I thought we reached a consensus when we discussed it before. I'll drop that part of the commit message, and we can discuss later. |
…meter type resolution AstGen provides all function call arguments with a result location, referenced through the call instruction index. The idea is that this should be the parameter type, but for `anytype` parameters, we use generic poison, which is required to be handled correctly. Previously, generic instantiations and inline calls worked by evaluating all args in advance, before resolving generic parameter types. This means any generic parameter (not just `anytype` ones) had generic poison result types. This caused missing result locations in some cases. Additionally, the generic instantiation logic caused `zirParam` to analyze the argument types a second time before coercion. This meant that for nominal types (struct/enum/etc), a *new* type was created, distinct to the result type which was previously forwarded to the argument expression. This commit fixes both of these issues. Generic parameter type resolution is now interleaved with argument analysis, so that we don't have unnecessary generic poison types, and generic instantiation logic now handles parameters itself rather than falling through to the standard zirParam logic, so avoids duplicating the types. Resolves: ziglang#16566 Resolves: ziglang#16258 Resolves: ziglang#16753
This bug was exposed by the previous commit, since array_init is now used for tuple parameters.
The changes to result locations and generic calls has caused mild changes to some compile errors. Some are slightly better, some slightly worse, but none of the changes are major.
This makes the reference trace appear for generic calls where it previously did not. Resolves: ziglang#16725
mlugg
force-pushed
the
result-type-shenanigans
branch
from
9ff80a9 to
f32b9bc
Compare
Vexu
added a commit
to Vexu/zig
that referenced
this pull request
Oct 9, 2023
This broke with ziglang#16604 but went unnoticed due to lack of tests. Closes ziglang#17444
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
See commit messages for details.
The final commit here has one small regression: the error return trace for
always_tailcalls isn't popped, so you could get a bogus line in your error return trace if a forced tail call returns an error and one of its parameters is an error set or union. The stars would really have to align to hit this in the wild, so it definitely seems like an acceptable regression for now since this PR otherwise fixes some real bugs.The solution to that regression is probably to move the error tracing logic into
analyzeCall, but that can be a future improvement.