Fix Issue 21288 & 10886 - incorrect this pointer #14969

RazvanN7 · 2023-03-08T15:15:09Z

It seems that when a function is called the, if it requires a context pointer, the compiler simply rewrites it to this.fcall even though the function that is being call sometimes explicitly provides it's aggregate. This leads to bogus messages and also it makes it impossible to take the address of such functions if inside a this context.

I am fixing this by requiring that the this pointers of the called function and the outer aggregate match.

dlang-bot · 2023-03-08T15:15:16Z

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 verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
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
✓	10886	normal	Typeof on @Property function triggers 'wrong this' type error
✓	21288	normal	Wrong context pointer for alias this function

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#14969"

compiler/src/dmd/expressionsem.d

ghost · 2023-03-08T23:59:36Z

compiler/src/dmd/expressionsem.d

+    if (!requiredAd)
+        return false;
+
+    if (thisAd == requiredAd)


wouldn't just is be enough here ?

Indeed, is would be preferred since we want to compare the pointers, but AST nodes do not define opEquals so we are fine with the default opEquals of object. Indeed, it would be slightly faster cause we are bypassing the the call to object.opEquals, but I doubt that this makes a performance difference in the release, inlined binary.

Long story-short: both is and == are fine in this case.

I dont see the problem with is, for the comment about coverage you are protective, i.e "in case of" but for this one you are not. "In case of" comparison overload would be used (at some point, even if unlikey), is is better.

dkorpel

My gut reaction to the diff was "this doesn't look right", but after reading it more carefully and reviewing it, it does look like a valid code change. Perhaps the documentation on haveSameThis could be clarified to make it easier to understand what's going on.

dkorpel · 2023-03-10T10:17:33Z

compiler/test/compilable/test19295.d

@@ -0,0 +1,10 @@
+struct S1(T...) {
+    auto fun() {
+        static assert(__traits(compiles, &T[0]));


This compiles now, but does the resulting delegate have the right this parameter?

But the result is not a delegate, it's a function, so there is no this pointer.

dkorpel · 2023-03-10T10:27:14Z

compiler/src/dmd/expressionsem.d

+ *  `true` if outerFunc and calledFunc may use the same `this` pointer.
+ * `false` otherwise.
+ */
+private bool haveSameThis(FuncDeclaration outerFunc, FuncDeclaration calledFunc)


This function doesn't look right. The logic in the function is surprisingly complex (many branches). I would expect haveSameThis to be commutative and take two arbitrary functions, but the names outerFunc and calledFunc suggest there are restrictions to the parameters which aren't documented.

Initially, I thought the fix is easy. I thought it would suffice to make sure that the this of the function containing the call and the this of the function being called are the same, however that does not cut it for derived member functions. Later on, I noticed that for nested functions there is some extra engineering done to get the right this. That's when I ended up with this function. I suspect that there are other places where this function might be used.

I don't think it can be made any simpler than that, but I am confident that what is in there at this point is correct.

dkorpel · 2023-03-10T10:31:17Z

compiler/src/dmd/expressionsem.d

+/*
+ * Check whether `outerFunc` and `calledFunc` have the same `this`.
+ * If `calledFunc` is the member of a base class of the class that contains
+ * `outerFunc` we consider that they have the same this.


The example makes it unclear what "have same this" means, it sounds more like "have covariant typeof(this)"

Well, I followed the existing naming pattern after getRightThis.

dkorpel · 2023-03-10T10:36:49Z

compiler/src/dmd/expressionsem.d

-        if (e.e1.op == EXP.error)
-            return setError();


Why is this removed?

Because it was added to fix a symptom of the bug this PR is fixing. Take a look at: #10123

RazvanN7 · 2023-03-10T11:14:30Z

@dkorpel I added some clarifications.

RazvanN7 · 2023-03-10T11:15:00Z

@dkorpel Thanks for the review.

ghost · 2023-06-25T18:36:26Z

This commit has introduced a regression, see https://issues.dlang.org/show_bug.cgi?id=24013.

kinke · 2023-08-25T14:19:37Z

I guess this also caused regression https://issues.dlang.org/show_bug.cgi?id=24109.

WalterBright · 2023-08-27T02:33:32Z

compiler/src/dmd/expressionsem.d

+ * `this` can be prepended to `fun`. When `gun` is called (it will result
+ * in an error, but that is not relevant here), which is a member of `X`,
+ * no `this` is needed because the outer function does not have the same
+ * `this` as `gun`.


But it will fail anyway because X.gun() does not supply a this to gun.

dlang-bot added the Severity:Bug Fix label Mar 8, 2023

ghost reviewed Mar 9, 2023

View reviewed changes

dkorpel approved these changes Mar 10, 2023

View reviewed changes

Fix Issue 21288 & 10886 - incorrect this pointer

30a91c2

RazvanN7 force-pushed the Issue_23761 branch from 8b5f6ee to 30a91c2 Compare March 10, 2023 11:14

RazvanN7 merged commit 141c568 into dlang:stable Mar 13, 2023

ibuclaw mentioned this pull request Mar 15, 2023

merge stable #14992

Merged

WalterBright reviewed Aug 27, 2023

View reviewed changes

Uh oh!

Fix Issue 21288 & 10886 - incorrect this pointer #14969

Fix Issue 21288 & 10886 - incorrect this pointer #14969

Uh oh!

Conversation

RazvanN7 commented Mar 8, 2023

Uh oh!

dlang-bot commented Mar 8, 2023

Bugzilla references

Testing this PR locally

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RazvanN7 Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Mar 10, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkorpel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RazvanN7 Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RazvanN7 commented Mar 10, 2023

Uh oh!

RazvanN7 commented Mar 10, 2023

Uh oh!

ghost commented Jun 25, 2023

Uh oh!

kinke commented Aug 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

RazvanN7 Mar 9, 2023 •

edited

Loading

ghost Mar 10, 2023 •

edited by ghost

Loading

RazvanN7 Mar 10, 2023 •

edited

Loading