fix Issue 14835 - Constant folding should not affect front end flow a…

[WalterBright]

…nalysis

Users should use static if if they need compile time conditional compilation.

[dlang-bot]

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	14835	major	Constant folding should not affect front end flow analysis

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#12311"

[WalterBright]

Blocked by dlang/druntime#3417

There'll likely be much more of this. Sigh.

[Geod24]

This does not fix the issue I reported. The reported issue was explicitly about static if.
I think this is a good fix, but not for issue 14835, rather for issue 13165.

[WalterBright]

@Geod24 I know, but it may provide the right solution to the static if issue.

[Geod24]

Could you add a description about how this fix the reported issue then ?

[WalterBright]

Use static if if you need compile time flow analysis, use if if you want run time flow analysis. It's not the right words, but the idea is clear.

[Geod24]

Suggested change

	void reachIf(bool x)()
	{
	if (!x)
	return;
	return;
	}
	bool reachIf(bool x)()
	{
	static if (!x)
	return false;
	int a;
	assert(a == 0);
	return true;
	}

The issue was explicitly about static if, but the test here only includes if.
Additionally, the extra statements are to guarantee that "Statement is not reachable" is triggered, which could stop happening to an empty return at the end of a void function due to unrelated changes.

[WalterBright]

The solution to the static if issue is to change the behavior of if. Hence the test is about if.

[Geod24]

@WalterBright : If I understand correctly, you intend to solve the issue by recommending people to switch from the following:

bool isEven(int i)()
{
   static if (i % 2) // static or not, the warning will get triggered if i is even
     return true;
   return false;
}

To the following:

bool isEven(int i)()
{
   if (i % 2) // static or not, the warning will get triggered if i is even
     return true;
   return false;
}

I think that makes sense, but it goes against many developer's instinct which is to reach for static if whenever they can.
Hence, it would be really great if such a fix could come with a mention in the website (spec or article) explaining what should be the best practice, otherwise someone else is bound to run in the same issue and report it.

[tsbockman]

The concept here seems right to me, although I can't comment on the implementation. I bumped the old forum thread for this issue just in case someone else sees a serious problem with this approach that we don't.

[WalterBright]

you intend to solve the issue by recommending people to switch from the following:

Yes.

[UplinkCoder]

I agree that an if is to be preferred to a static if whenever possible.
I am not sure however if you want unreachable warnings to be ignore for static branches though.
Seems like it could have unintended consequences.

[UplinkCoder]

src/object.d(2215): Error: function object.ModuleInfo.namenoreturn exp;orassert(0); at end of function
yep. sure has unintended consequences.

[WalterBright]

src/object.d(2215): Error: function

There's a PR for that: dlang/druntime#3417

[schveiguy]

If I understand this reasoning properly (didn't even look at the changes, I don't understand DMD), this doesn't fix the issue.

There are reasons to use static if vs if. For instance, static if forces CTFE for a function call, and also does not introduce a scope.

And in actuality, as I said in the bug report, the "code is unreachable" statement is incorrect. It's reachable, just not in this instantiation.

This is going to result in things like:

static if(doSomethingAtCTFE())
{
   if(1) return retval; // trick the compiler
}

Which I don't think is a good look.

What happens when foreach on a CT tuple is used? That's another common source of spurious "unreachable" errors.

[schveiguy]

My previous comment notwithstanding, as long as the if(1) trick works, this at least gives a path to working around the issue, ugly as it is.

[tsbockman]

@schveiguy

There are reasons to use static if vs if. For instance, static if forces CTFE for a function call, and also does not introduce a scope.

Although annoying, "not reachable" warnings caused by static if(predicate) statements can generally be worked around by adding else or static if(!predicate) as needed. Also, enum can be used to force CTFE function calls prior to a runtime if.

By contrast, there is no simple workaround for some of the spurious warnings that could theoretically be triggered by runtime control flow: #5229 (comment)

(I'm not sure about static foreach - I don't think it was available yet when I last studied all of this.)

[WalterBright]

the "code is unreachable" statement is incorrect. It's reachable, just not in this instantiation.

That depends on your perspective. From mine, static if inserts code. If that insert makes other code unreachable, it is unreachable.

This is different from the if version. Whether the condition always evaluates to true or false depends on how far the compiler goes with constant folding, hence it is inherently unreliable. Therefore, it makes sense to not do flow analysis based on the condition.

[thewilsonator]

this also probably needs a force push in order to restart all of the CIs

[MoonlightSentinel]

This is different from the if version. Whether the condition always evaluates to true or false depends on how far the compiler goes with constant folding, hence it is inherently unreliable.

-preview=fieldwise is actually a good example of this problem because it enables dmd to const-fold == for structs without members. This has already caused problems for phobos which should work with and without that switch during the transition period.

[WalterBright]

Now it's blocked by dlang/phobos#8180

[RazvanN7]

This seems to be green now. Should we merge this?

[dkorpel]

It's still not clear whether this fix satisfies users (See Steven's comment #12311 (comment)), but if we go with "Constant folding should not affect front end flow analysis", then flow analysis of do {} while () and for () should get the same treatment. They consider constant-folded loop conditions, and this PR doesn't change that currently.

[schveiguy]

It's still not clear whether this fix satisfies users

I don't love that static if will still complain about unreachable code, but if there is a workaround like I said in #12311 (comment), then we can consider the issue resolved, and perhaps open another issue on making that workaround obsolete.

Note the original title of the bug was "Statement is not reachable doesn't play along generic code", which describes more accurately the problem I was having, and would not be fixed by this PR.

[WalterBright]

I'm not sure if the rebase was done correctly. Have to re-evaluate this.