Fix Issue 24246 - This eliminates the ICE, stopping the bleed, but do…

[maxhaton]

…es not fix the underlying issue in that the type checking is faulty.

[dlang-bot]

Thanks for your pull request, @maxhaton!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	24246	critical	ICE: CTFE internal error: literal 'TypeExp' in src/dmd/ctfeexpr.d(439)

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 "stable + dmd#15818"

[ibuclaw]

Use fatal() instead.

dmd/compiler/src/dmd/cppmangle.d

Line 486 in 8c84af8

.error(ti.loc, "%s `%s` internal compiler error: C++ `%s` template value parameter is not supported", ti.kind, ti.toPrettyChars, tv.valType.toChars());

[maxhaton]

I don't know if it should be completely fatal while it triggers so easily

[ibuclaw]

It took years to find it. :-)

[ibuclaw]

The one I linked is trivial to hit as well. Mix any D type (arrays) with extern C++.

[maxhaton]

Is it realistic to recover from that in an IsExp (say)?

[ibuclaw]

It's an internal error, you're not meant to be able to recover.

[ibuclaw]

To be clear, this ICE is a result of a failure of semantic to analyse condexps properly.

[maxhaton]

Its effectively a type check done way too late is my thinking. It should be fatal when the code actuall works but its not CondExps as much as a vast surface area of the type checking being totally bogus.

[ghost]

When does a primary exp such as int is accepted since ?

It is like https://issues.dlang.org/show_bug.cgi?id=24229.

[ghost]

INitially that comes from an alias presumably ? IdentExp -> AliasDecl -> TypeExp ? Even if apparently it is also accepted as-is.

[RazvanN7]

I don't see the point in merging this. The assert(0) technically should never be reached and is put there as a precautionary measure. Deleting it and turning it into a normal error is only going to make it harder in the future to track where the issue is coming from. I suggest you either fix the issue or leave it as is - using fatal or error adds no benefit in my opinion.

[maxhaton]

I don't see the point in merging this. The assert(0) technically should never be reached and is put there as a precautionary measure. Deleting it and turning it into a normal error is only going to make it harder in the future to track where the issue is coming from. I suggest you either fix the issue or leave it as is - using fatal or error adds no benefit in my opinion.

Currently the assert(0) just makes the compiler blow up without even an ICE message. It's a bit lazy.

It should not be reachable at all, by construction, it's the type checker that's broken. Fixing that will not be particularly trivial, this effectively steps in as a bit of post-hoc type checking. The fact that it says internal error has been maintained but is honestly kind of meaningless.

[RazvanN7]

@maxhaton That's preferable because turning it into an error will make it liable to error gagging. Consider the case where the problematic code is triggered from a typeof expression. What will happen: you will mask the error and continue compilation (and if fatal is used, it will seem that compilation has exited gracefully but no binary was created).

That's why I think that we should leave the assert as is until we fix the issue. Turning it into an error really doesn't buy as anything, whereas the assertion will make users submit bug reports with cases that we need to fix.