Fix Issue 19181: always error when an UFCS or opDispatch call has syntactically invalid arguments #8584

FeepingCreature · 2018-08-20T06:45:36Z

Repro:

$ dmd test.d
---
module test;

struct S {
    void opDispatch(string name, T)(T arg) { }
}

void main() {
    S s;
    s.foo(LanguageError);
}

Before:
test.d(9): Error: no property foo for type S

After:
test.d(9): Error: undefined identifier LanguageError

dlang-bot · 2018-08-20T06:45:38Z

Thanks for your pull request and interest in making D better, @FeepingCreature! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

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

Auto-close	Bugzilla	Severity	Description
✓	19181	normal	Semantic errors in opDispatch argument lead to "no property X"

Testing this PR locally

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

dub fetch digger
dub run digger -- build "master + dmd#8584"

FeepingCreature · 2018-08-20T06:50:31Z

Holy hell, that's the fastest approval I have ever gotten on Github, let alone DMD.

jacob-carlborg

Needs to have a test.

JinShil · 2018-08-20T06:51:53Z

Doesn't this need a test case to prevent a regression.

jacob-carlborg · 2018-08-20T06:52:02Z

Holy hell, that's the fastest approval I have ever gotten on Github, let alone DMD.

Sorry, I had to change my mind.

jacob-carlborg · 2018-08-20T06:53:11Z

Doesn't this need a test case to prevent a regression.

Yes, I changed my mind and requested changes.

FeepingCreature · 2018-08-20T06:53:12Z

The original behavior is not strictly speaking wrong, it's just highly misleading. opDispatch does fail to match since the call fails to compile - it just fails to compile due to a call-site error. That said, I'm not sure how you test for a specific error message.

edit: The test failure is in master unchanged. God damnit. Why do we even have tests.

JinShil · 2018-08-20T06:54:56Z

That said, I'm not sure how you test for a specific error message.

See examples in the test/fail_compilation directory for examples. Also see TEST_OUTPUT described in the Readme at https://github.com/dlang/dmd/tree/master/test

FeepingCreature · 2018-08-20T06:58:21Z

Thanks. Test added.

jacob-carlborg · 2018-08-20T06:59:40Z

test/fail_compilation/fail19181.d

+fail_compilation/fail19181.d(13): Error: undefined identifier LanguageError
+---
+*/
+struct S {


Please follow the standard D style, curly braces on their own lines.

JinShil · 2018-08-20T07:04:10Z

test/fail_compilation/fail19181.d

+/*
+TEST_OUTPUT:
+---
+fail_compilation/fail19181.d(15): Error: undefined identifier LanguageError


I think the tests suite will probably fail because LanguageError needs to be surrounded with tick marks. We'll find out soon enough.

Since the other tests have that too, I've just gone ahead and added it. Can't hurt.

jacob-carlborg · 2018-08-20T07:06:23Z

Back to approved, still less than 20 minutes 😃.

FeepingCreature · 2018-08-20T07:06:39Z

Still the fastest, I believe!

FeepingCreature · 2018-08-20T08:02:25Z

Again, that is not an error in my change.

JinShil · 2018-08-20T08:05:22Z

Again, that is not an error in my change.

Are you sure? Run the test suite locally with make -C test -f Makefile. Or to run just the failing test use rdmd test/run.d test/fail_compilation/ice14929.d.

FeepingCreature · 2018-08-20T08:20:45Z

Are you sure?

Yes I am, given that it happens on master. 😄

edit: I AM AN IDIOT who doesn't understand how fail tests work. Thanks! This is definitely at least a change from my code.

edit: Hah! The test was wrong.

edit: Specifically, the body of the typeof was semantically invalid, which due to the change errored before the add() internal error hit, which made the typeof() evaluate to false, as it should.

…valid arguments, always error.

jacob-carlborg · 2018-08-20T12:07:44Z

test/fail_compilation/ice14929.d

@@ -75,7 +75,7 @@ template isComponentStorage(CS, C)
    {
        CS cs = CS.init;
        ulong eid;
-        cs.add(eid, c);
+        cs.add(eid, C.init);


Why was this change necessary?

Fun reason: Previously, DMD would try to semantically evaluate cs.add before figuring out what if anything c is, hit the !in instantiation error, and bail with the TEST_OUTPUT message. WIth this PR's change, it verifies the arguments to cs.add() first and immediately notices that there is no such variable as c, causing the typeof to fail and, as is typeof's wont, silently return false. Since I can only presume that the fact that c doesn't exist is not actually the point of this test, since there's no comment and it's unrelated to the error message, I just went ahead and made it semantically valid.

Ah, yes, of course. I was reading the corrected code and tried to figure out what was wrong with it 😃.

It is a bit weird and inconsistent that typeof errors out if only the error that happens in typeof is in a different template.

But that's neither what this bug nor the testcase's bug is about, so ... 🤷‍♂️

wilzbach · 2018-08-20T17:33:16Z

Or to run just the failing test use rdmd test/run.d test/fail_compilation/ice14929.d.

FYI: for the lazy people like me: ./test/run.d fail_compilation/ice14929.d and a few other combinations work too.

…ror-on-invalid-arguments-in-ufcs-opDispatch Fix Issue 19181: always error when an UFCS or opDispatch call has syntactically invalid arguments merged-on-behalf-of: Sebastian Wilzbach <sebi.wilzbach@gmail.com>

MoonlightSentinel · 2020-01-08T17:03:31Z

This introduced a regression: https://issues.dlang.org/show_bug.cgi?id=20488

FeepingCreature · 2020-01-15T09:13:44Z

(Regression fixed.)

dlang-bot added the Severity:Bug Fix label Aug 20, 2018

jacob-carlborg approved these changes Aug 20, 2018

View reviewed changes

jacob-carlborg suggested changes Aug 20, 2018

View reviewed changes

FeepingCreature force-pushed the fix/Issue-19181-error-on-invalid-arguments-in-ufcs-opDispatch branch from 126b2df to 0534fee Compare August 20, 2018 06:58

jacob-carlborg suggested changes Aug 20, 2018

View reviewed changes

FeepingCreature force-pushed the fix/Issue-19181-error-on-invalid-arguments-in-ufcs-opDispatch branch from 0534fee to 56ea52d Compare August 20, 2018 07:02

jacob-carlborg approved these changes Aug 20, 2018

View reviewed changes

JinShil reviewed Aug 20, 2018

View reviewed changes

FeepingCreature force-pushed the fix/Issue-19181-error-on-invalid-arguments-in-ufcs-opDispatch branch from 56ea52d to 52ee235 Compare August 20, 2018 07:05

Fix Issue 19181: when an UFCS or opDispatch call has syntactically in…

4ea3485

…valid arguments, always error.

FeepingCreature force-pushed the fix/Issue-19181-error-on-invalid-arguments-in-ufcs-opDispatch branch from 52ee235 to 4ea3485 Compare August 20, 2018 09:35

jacob-carlborg reviewed Aug 20, 2018

View reviewed changes

wilzbach approved these changes Aug 20, 2018

View reviewed changes

wilzbach added the Merge:auto-merge label Aug 20, 2018

dlang-bot merged commit c2317ad into dlang:master Aug 20, 2018

FeepingCreature mentioned this pull request Jan 9, 2020

Fix issue 20488: opDispatch: copy call expression before checking for semantically valid parameters #10718

Merged

FeepingCreature deleted the fix/Issue-19181-error-on-invalid-arguments-in-ufcs-opDispatch branch January 15, 2020 09:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Issue 19181: always error when an UFCS or opDispatch call has syntactically invalid arguments #8584

Fix Issue 19181: always error when an UFCS or opDispatch call has syntactically invalid arguments #8584

FeepingCreature commented Aug 20, 2018 •

edited

Loading

dlang-bot commented Aug 20, 2018

FeepingCreature commented Aug 20, 2018

jacob-carlborg left a comment

JinShil commented Aug 20, 2018

jacob-carlborg commented Aug 20, 2018

jacob-carlborg commented Aug 20, 2018

FeepingCreature commented Aug 20, 2018 •

edited

Loading

JinShil commented Aug 20, 2018

FeepingCreature commented Aug 20, 2018 •

edited

Loading

jacob-carlborg Aug 20, 2018

FeepingCreature Aug 20, 2018

JinShil Aug 20, 2018

FeepingCreature Aug 20, 2018

jacob-carlborg commented Aug 20, 2018 •

edited

Loading

FeepingCreature commented Aug 20, 2018

FeepingCreature commented Aug 20, 2018

JinShil commented Aug 20, 2018

FeepingCreature commented Aug 20, 2018 •

edited

Loading

jacob-carlborg Aug 20, 2018

FeepingCreature Aug 20, 2018 •

edited

Loading

jacob-carlborg Aug 20, 2018

FeepingCreature Aug 20, 2018

wilzbach commented Aug 20, 2018

MoonlightSentinel commented Jan 8, 2020

FeepingCreature commented Jan 15, 2020

Fix Issue 19181: always error when an UFCS or opDispatch call has syntactically invalid arguments #8584

Fix Issue 19181: always error when an UFCS or opDispatch call has syntactically invalid arguments #8584

Conversation

FeepingCreature commented Aug 20, 2018 • edited Loading

dlang-bot commented Aug 20, 2018

Bugzilla references

Testing this PR locally

FeepingCreature commented Aug 20, 2018

jacob-carlborg left a comment

Choose a reason for hiding this comment

JinShil commented Aug 20, 2018

jacob-carlborg commented Aug 20, 2018

jacob-carlborg commented Aug 20, 2018

FeepingCreature commented Aug 20, 2018 • edited Loading

JinShil commented Aug 20, 2018

FeepingCreature commented Aug 20, 2018 • edited Loading

jacob-carlborg Aug 20, 2018

Choose a reason for hiding this comment

FeepingCreature Aug 20, 2018

Choose a reason for hiding this comment

JinShil Aug 20, 2018

Choose a reason for hiding this comment

FeepingCreature Aug 20, 2018

Choose a reason for hiding this comment

jacob-carlborg commented Aug 20, 2018 • edited Loading

FeepingCreature commented Aug 20, 2018

FeepingCreature commented Aug 20, 2018

JinShil commented Aug 20, 2018

FeepingCreature commented Aug 20, 2018 • edited Loading

jacob-carlborg Aug 20, 2018

Choose a reason for hiding this comment

FeepingCreature Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

jacob-carlborg Aug 20, 2018

Choose a reason for hiding this comment

FeepingCreature Aug 20, 2018

Choose a reason for hiding this comment

wilzbach commented Aug 20, 2018

MoonlightSentinel commented Jan 8, 2020

FeepingCreature commented Jan 15, 2020

FeepingCreature commented Aug 20, 2018 •

edited

Loading

FeepingCreature commented Aug 20, 2018 •

edited

Loading

FeepingCreature commented Aug 20, 2018 •

edited

Loading

jacob-carlborg commented Aug 20, 2018 •

edited

Loading

FeepingCreature commented Aug 20, 2018 •

edited

Loading

FeepingCreature Aug 20, 2018 •

edited

Loading