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

Early returns in NullCoal #10791

Merged
merged 7 commits into from
Nov 15, 2022
Merged

Conversation

RblSb
Copy link
Member

@RblSb RblSb commented Sep 6, 2022

Closes #10771

src/typing/typer.ml Outdated Show resolved Hide resolved
@RblSb RblSb requested a review from Simn November 2, 2022 00:21
@Simn
Copy link
Member

Simn commented Nov 2, 2022

That has_dead_end function is still not correct. It has to account for any sub-expression that is evaluated unconditionally, which is almost all of them.

Also | TWhile (_, body, DoWhile) -> has_dead_end body isn't right because a TBreak | TContinue in the loop body does not terminate the expression as a a whole.

Note that this is also true for a TReturn inside a TFunction. Technically, this could also be true for a TThrow inside a TTry, but we can't tell that at compile-time in all cases.

@Simn Simn self-assigned this Nov 10, 2022
@Simn
Copy link
Member

Simn commented Nov 10, 2022

I took a look at that function myself. Your test cases made me realize that this is trickier than I thought in some regards. I left some comments, please see if this makes sense to you!

I didn't check how this affects the case in nullSafety.ml where this function is called. I'm actually surprised that it's only called in one place there, because termination status should affect null safety in several ways.

@Simn
Copy link
Member

Simn commented Nov 10, 2022

Also I remembered that somebody added check_expr some time ago, which is ideal for recursing in loops like this.

@RblSb
Copy link
Member Author

RblSb commented Nov 10, 2022

Thank you for help, it was too complicated for me. Glad my test cases were helpful.
Everything seems ok for me, but i should report minor thing:

eq("Null<Int>", typeString(v ?? {
	for (i in 0...1) {
		return;
	}
	v;
}));

This shows on hover Null<Int>, but resolves to Int in typeExpr macro and in test result, probably because optimization. Is this fine?
Also not sure if we need to check TFor(_, cond, _). Is this for inline iterators and just to be sure?

@Simn
Copy link
Member

Simn commented Nov 10, 2022

Yes that's probably from loop unrolling.

The cond check is for something like for (i in [return]).

@Simn
Copy link
Member

Simn commented Nov 11, 2022

Actually that TFor handling isn't safe for something like this I think:

for (i in [a, return]) {
    break;
}

So it's better to ignore the condition after all.

@Aurel300
Copy link
Member

Actually that TFor handling isn't safe for something like this I think:

for (i in [a, return]) {
    break;
}

So it's better to ignore the condition after all.

I don't understand this: [a, return] is evaluated first, so the expression you wrote definitely returns, without a single iteration of the loop. loop e1 for the TFor case makes sense to me.

@Simn
Copy link
Member

Simn commented Nov 13, 2022

You're right, I applied some sort of loop unrolling in my head, but that's not what actually happens evaluation-wise.

@Simn Simn merged commit 3177901 into HaxeFoundation:development Nov 15, 2022
@Simn
Copy link
Member

Simn commented Nov 15, 2022

Thank you for your patience!

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.

Type of foo ?? throw bar is Null<Foo> instead of Foo
3 participants