[REG2.065a] Issue 11822 - -de switch causees ICE with auto return and other stuff

[9rnsr]

https://d.puremagic.com/issues/show_bug.cgi?id=11822

This is caused by #2850.

TypeFunction is also used for the extra payload of function attributes. Therefore, even if function signature or its body has errors, we would be better to leave that fd->type->ty == Tfunction.

[9rnsr]

Updated, and auto tester shows green now.

@WalterBright , this is contrary change against your error propagation fixes - #2753 and #2850. Can you agree with the reason?

[WalterBright]

I don't agree with this change, because if we're ever going to make trySemantic and friends work right we have to do proper error propagation. This regression is caused by an overlooked case.

[9rnsr]

I considered your argument, but again concluded that setting FuncDeclaration::type to Type::terror is rather harmful than benefit.

Currently many function-related attributes are stored in TypeFunction - purity, safety, parameter count, parameter types, parameter names, default arguments, varargs, etc. Once error occurs, normal path code which touch the attributes won't work anymore and we should specially treat "error function" there. In other words, most of the places that currently asserts (ty == Tfunction) should be rechecked for the error path. It would be huge cost.

If we keep fd->type->ty == Tfunction, the special handling would be avoided. We can rely on the invariance, and we would keep compiler code simple. I think it is much benefit. In other words:

This regression is caused by an overlooked case.

We can make the overlooked case count zero. This change makes it.

Furthermore, not so many places would require the conversion from tf with tf->next == Type::terror to Type::terror or ErrorExp for the "better error propagation". For that, this PR now fixes TypePointer::semantic and TypeDelegate::semantic. Maybe we should also look VarExp, SymOffExp, CallExp, etc, but we don't have to do it now.

[WalterBright]

The idea with error propagation is that if I do something like:
e = e->semantic(sc);
that e will be guaranteed to be ErrorExp if and only if any errors occurred during semantic(). The global.errors should not need to be checked. The changes I made are necessary to support that. I overlooked a case, but so far it is the only overlooked one that has emerged.

[9rnsr]

I'm not against the idea. What I'm saying is, we should continue to keep fd->type->ty == Tfunction, even if error occurs in the body of fd.

If error occurs in the body of fd, currently fd->type is immediately replaced to Type::terror. Therefore, in the issue case FuncDeclaration::isPure must handle the error case specially. This is a place that you had overlooked.

And, many other places FuncDeclaration::isSafe, FuncDeclaration::isTrusted, FuncDeclaration::getParameters, ... would have same problem. So you should fix all of them as same as isPure. You can search the places which may be broken implicitly by grepping type->ty == Tfunction. Currently 100 over places exist. Did you see the all places? Can you check them all?

Again, I'm not against for the error propagation itself. I'm saying that your changed had silently broken the assumptions for the internal AST in many semantic processing code. This change is low cost than adjusting over 100 code places.

[yebblies]

We should be storing the results of purity/safety/etc inference in the declaration, not the type, and constructing the correct type at the end, right? Then we could safely set the type to Type::terror...

[9rnsr]

@yebblies But two function types void() and void() pure are not same. So your idea would need more and more compiler change.

[yebblies]

Oh, we'll need to store that stuff in the function type too. But isPure/isSafe/everything else should be looking at the declaration for that information, not the type.

This is just a long-term idea, I expect it will take quite a bit of work to accomplish.

[9rnsr]

I don't feel good that idea. It would just add duplication between function declaration and its type. Sync them during semantic processing would be essentially extra cost.

[yebblies]

We already have had lots of trouble with function types: parameter names, default parameters, purity/safety inference changing the type etc I guess we'll see what it looks like if/when I get around to it.

[9rnsr]

The troubles were mostly fixed already:

• Parameter names and default arguments should be stored in function type, to allow function pointer/delegate type with parameter names/default args.
• Function type of the function with inference should be committed after its semantic3 completed.

[yebblies]

Parameter names and default arguments should be stored in function type, to allow function pointer/delegate type with parameter names/default args.

I don't agree with this, but we're getting further off-topic now.

[9rnsr]

we're getting further off-topic now.

Right. So, how do you think about this change?

Currently once fd->type is set to terror, we cannot the attributes which had stored in fd->type anymore. Therefore, all of function attribute accesses should have special branch for the case fd->type-> == Terror. I think that the way will make compiler code complex without any benefits.

Instead, I think keeping the invariance fd->type->ty == Tfunction, even if fd has errors, would have much more benefit.

[yebblies]

Instead, I think keeping the invariance fd->type->ty == Tfunction, even if fd has errors, would have much more benefit.

There are two invariants that are in conflict here:

• fd->type->ty == Tfunction
• fd->type == Type::terror if fd has errors

Breaking either one has problems.

Right. So, how do you think about this change?

I think we need to fix the ICE. If another solution along Walter's suggested line isn't forthcoming, then this will do it.

[9rnsr]

There are two invariants that are in conflict here:

• fd->type->ty == Tfunction
• fd->type == Type::terror if fd has errors

This change removes the second invariant intentionally. For error propagation, we can use fd->errors or fd->type->nextOf() == Type::terror.

[WalterBright]

This fixes it:

expression.c
2294a2295,2296
>           if (outerfunc->type->ty == Terror)
>               return;
2312a2315,2316
>           if (calledparent->type->ty == Terror)
>               return;
9065c9069
<         if (!f)
---
>         if (!f || f->type->ty == Terror)

[9rnsr]

@WalterBright I already considered your change. Yes, it would fix the reported ICE. But other ICE possibilities is still there. For example, FuncDeclaration::isSafe() is called on error function, it would cause ICE as same as issue 11822. Can you argue it won't occur during semantic?

[WalterBright]

@9rnsr if it does occur it will be fixed. I think I got most of them, but wouldn't be surprised if there are a couple more lingering ones.

[9rnsr]

Yes, we will be able to fix the found bugs. But it is fragile and needs more work.

My main point is, stateless code is more robust than stateful code. If we continue current way (fd->type may become Type::terror), we should handle the state in various places, like your fix. On the other hand, this way is more better because we can rely on the invariant that fd->type->ty == Tfunction. Normal semantic path and error path would share the code. Of course we can add more code for better error handling. But the code will guarantee the minimum behavior even if the additional fix does not exist.

Today, dmd project does not have so many contributors. So we should continue to reduce code complexity to reduce bug possibility. But, your way would need more man-power. It is definitely not good.

[WalterBright]

@9rnsr we are going to disagree on this one. I believe my approach will work better in the long term, and there's been only regression caused by it (and easily fixed), so I don't see anything fundamentally wrong with it.

I agree with your goal of stateless code, and I believe that propagating errors rather than setting global.errors is the right way to go, and the changes I suggested are the most straightforward path to that.

[9rnsr]

@WalterBright I think you should recognize that function is very special declaration than other declarations which has their own types. (VarDeclaration, StructDeclaration, etc.)

As I repeatedly explained, FuncDeclaration and TypeFunction is closely related each other. So, setting type to terror does not fit only for FuncDeclaration. (Note that I don't argue that the approach does not fit to other declarations.)

And, fd->type is never directly used for the expression type. All of function types always appear in expressions via function pointer type or delegate type. So, the approach does not have so many benefit in error propagation mechanism.

Walter, I also like making things simple. But too simplicity would make other things more complex. I believe that this change will reduce total complexity in front-end layer. I hope you don't add future unnecessary work.

[WalterBright]

@9rnsr I understand your points, I just do not agree with the conclusion.

[9rnsr]

When I check issue 11850, I found that the reason of segfault is same as this. (The bug itself was caused by merging #3020, but the segfault is caused by accessing TypeFunction::parameters access without checking fd->type->ty == Terror).
And I confirmed that this change also fix 11850. I'll start to shrink test case for the new issue and adding it in this PR.

Walter, the change you proposed does not fix 11850. Do you think about that?

[9rnsr]

Added test case.

[WalterBright]

With the change I proposed, it does fix 11850. Here is the output:

C:\cbx\mars>dmd foo
DMD v2.065 DEBUG
foo.d(3): Error: incompatible types for ((a) < ([0])): 'uint[]' and 'int[]'
c:\cbx\mars\phobos\std\algorithm.d(1356):        instantiated from here: FilterResult!(__lambda1, ui
nt[][])
foo.d(3):        instantiated from here: filter!(uint[][])
c:\cbx\mars\phobos\std\algorithm.d(1393): Error: template D main.__lambda1 does not match any functi
on template declaration. Candidates are:
foo.d(3):        foo.main.__lambda1
c:\cbx\mars\phobos\std\algorithm.d(1393): Error: template foo.main.__lambda1 cannot deduce template
function from argument types !()(uint[])
foo.d(3): Error: template instance foo.main.filter!((a) => a < [0]).filter!(uint[][]) error instanti
ating
foo.d(3): Error: template std.algorithm.filter does not match any function template declaration. Can
didates are:
c:\cbx\mars\phobos\std\algorithm.d(1352):        std.algorithm.filter(alias pred) if (is(typeof(unar
yFun!pred)))
foo.d(3): Error: template std.algorithm.filter(alias pred) if (is(typeof(unaryFun!pred))) cannot ded
uce template function from argument types !((a) => a < [0])(uint[][])
c:\cbx\mars\phobos\std\algorithm.d(1415): Error: template D main.__lambda1 does not match any functi
on template declaration. Candidates are:
foo.d(3):        foo.main.__lambda1
c:\cbx\mars\phobos\std\algorithm.d(1415): Error: template foo.main.__lambda1 cannot deduce template
function from argument types !()(uint[])

Though it is possible that my fork is a bit behind yours.

[9rnsr]

Added test case for issue 11857.

[9rnsr]

@WalterBright I believe this approach is better for the long-term development of dmd. I'll continue to maintain this PR.

But, on the other hand, we should fix the ICE regression for 2.065 release.

Could you open another PR that implements which based on your approach? If the PR passes auto tester, I'll merge it.

[9rnsr]

@WalterBright Ping?

[WalterBright]

@9rnsr I'll do it.

[9rnsr]

Added test case for issue 11922.

[WalterBright]

This appears to be setting nextOf() to terror instead of the type itself. I still disagree with this - any type that has components that are terror should themselves be replaced with terror.

[9rnsr]

any type that has components that are terror should themselves be replaced with terror.

I still think that TypeFunction would be an exception. But I seem that the current approach is working so so. So I close this.