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

Fix null coal assign #11963

Open
wants to merge 12 commits into
base: development
Choose a base branch
from
Open

Conversation

RblSb
Copy link
Member

@RblSb RblSb commented Jan 31, 2025

This types e1 twice, maybe can be done without it?
Closes #11931

@Simn
Copy link
Member

Simn commented Jan 31, 2025

We should never double-type anything, so it would be a good idea to add a test that checks that. One way to do that is by having a macro call that would execute twice.

It would also be good to add a test that's a bit more straightforward.

@skial skial mentioned this pull request Feb 3, 2025
1 task
@Simn
Copy link
Member

Simn commented Feb 4, 2025

I think this case is quite complicated and we'll have to do something similar to what type_assign_op does. In those cases, we have a write = read op rhs situation, while here it's if (read) write = rhs.

@Simn
Copy link
Member

Simn commented Feb 4, 2025

I've started implementing this based on the current tests. We'll have to go through all these AK-variants to see what else can come up here, and test/implement it accordingly.

This approach is going to lead to some code duplication with type_assign_op, but we can look into that later.

@Simn
Copy link
Member

Simn commented Feb 5, 2025

Of note: testing the correct code generation isn't very trivial because the difference between null ? set(newval) : val and set(null ? newval : val) isn't very observable unless we can track the set-operation. This means that we should add an optimization test for all cases to make sure we're generating the correct code.

eq("value", value);

// TODO: this fails at the moment with some "not enough arguments error"
// mutAssignLeft(obj.field) ??= "not value";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails while typing the lhs already, so the whole ??= handling isn't even involved here. I think what happens is that it somehow tries to resolve the call while typing the macro result in MSet mode, or something along those lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Null-coalesce assign should not perform assign if not null
2 participants