Fix Issue 20800 - Error: cannot implicitly convert expression with bad diagnostic #11106
Conversation
|
Thanks for your pull request and interest in making D better, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#11106" |
|
The C++ interop failures seems unrelated: a wget failure to download. |
src/dmd/hdrgen.d
Outdated
| commaExtract = ce.e2; | ||
| else if (auto se = exp.isStructLiteralExp) | ||
| commaExtract = se; | ||
| expToBuffer(commaExtract, precedence[exp.op], buf, hgs); |
There was a problem hiding this comment.
this seems to segault, with null commaExtract I'm guessing.
There was a problem hiding this comment.
Yep, apparently there are some situations in phobos where the compilation of code leads to .toChars on a comma expression, maybe when running with -deps. This might be problematic, we'll see what the tester says.
src/dmd/hdrgen.d
Outdated
| // CommaExp that contain temporaries inserted via | ||
| // `copyToTemp` are usually of the form | ||
| // ((T __temp = exp), __tmp) | ||
| if (ve && ve.var.storage_class & STC.temp) |
There was a problem hiding this comment.
// for all other cases, go on the old path
if (!ve || !ve.var.storage_class & STC.temp)
visit(cast(BinExp)e);
...
|
ping. |
|
@RazvanN7 This seems to have introduced a regression. I am still reducing the testcase. |
| // Asserts are here to easily spot | ||
| // missing cases where CommaExp | ||
| // are used for other constructs | ||
| auto vd = ve.var.isVarDeclaration(); |
There was a problem hiding this comment.
The comment says you are unsure about this.
In this case don't make it an assert.
Assertion is defined as: a statement that you strongly believe is true.
There was a problem hiding this comment.
Yes, I am aware that the assert is not always true, but on the other hand I have no clue what other possible forms the CommaExp might have in this particular scenario, therefore, instead of letting a potential segfault with no additional information happen, I opted for an assert that gives you the exact line that is causing problems.
|
An If doing a proper ice would been better.
But I see now why you put the assert.
It's a reasonable choice I guess.
…On Mon, Aug 3, 2020, 5:18 PM Razvan Nitu ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/dmd/hdrgen.d
<#11106 (comment)>:
> +
+ // not a CommaExp introduced for temporaries, go on
+ // the old path
+ if (!ve || !ve.var.storage_class & STC.temp)
+ {
+ visit(cast(BinExp)e);
+ return;
+ }
+
+ // CommaExp that contain temporaries inserted via
+ // `copyToTemp` are usually of the form
+ // ((T __temp = exp), __tmp).
+ // Asserts are here to easily spot
+ // missing cases where CommaExp
+ // are used for other constructs
+ auto vd = ve.var.isVarDeclaration();
Yes, I am aware that the assert is not always true, but on the other hand
I have no clue what other possible forms the CommaExp might have in this
particular scenario, therefore, instead of letting a potential segfault
with no additional information happen, I opted for an assert that gives you
the exact line that is causing problems.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#11106 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAVWSCG54WERGWYKFHQL7ZTR63IKVANCNFSM4M34P43Q>
.
|
CommaExps are used internally in the compiler in a lot of places, but they shouldn't be exposed to the user. Since expression printing is done in thehdrgenfile, I added a new checkpoint where all comma expressions are captured and transformed into user readable form. The patch may be incomplete since I tackled only the cases where temporaries are inserted into CommaExp, but we can add to the function more cases as soon as they pop up.I forgot if error fixing PRs should go to stable or not.