Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 67 additions & 4 deletions compiler/src/dmd/expressionsem.d
Original file line number Diff line number Diff line change
Expand Up @@ -1155,6 +1155,69 @@ L1:
return e1;
}

/*
* 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I followed the existing naming pattern after getRightThis.

*
* This function is used to test whether `this` needs to be prepended to
* a function call or function symbol. For example:
*
* struct X
* {
* void gun() {}
* }
* struct A
* {
* void fun() {}
* void sun()
* {
* fun();
* X.gun(); // error
* }
* }
*
* When `fun` is called, `outerfunc` = `sun` and `calledFunc = `fun`.
* `sun` is a member of `A` and `fun` is also a member of `A`, therefore
* `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`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
auto thisAd = outerFunc.isMemberLocal();
if (!thisAd)
return false;

auto requiredAd = calledFunc.isMemberLocal();
if (!requiredAd)
return false;

if (thisAd == requiredAd)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't just is be enough here ?

Copy link
Contributor Author

@RazvanN7 RazvanN7 Mar 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@ghost ghost Mar 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return true;

// outerfunc is the member of a base class that contains calledFunc,
// then we consider that they have the same this.
auto cd = requiredAd.isClassDeclaration();
if (!cd)
return false;

if (cd.isBaseOf2(thisAd.isClassDeclaration()))
return true;

// if outerfunc is the member of a nested aggregate, then let
// getRightThis take care of this.
if (thisAd.isNested())
return true;

return false;
}

/***************************************
* Pull out any properties.
*/
Expand Down Expand Up @@ -5209,7 +5272,8 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
if (exp.f.checkNestedReference(sc, exp.loc))
return setError();

if (hasThis(sc))
auto memberFunc = hasThis(sc);
if (memberFunc && haveSameThis(memberFunc, exp.f))
{
// Supply an implicit 'this', as in
// this.ident
Expand Down Expand Up @@ -6892,8 +6956,6 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
AggregateDeclaration ad = f.isMemberLocal();
if (f.needThis())
e.e1 = getRightThis(e.loc, sc, ad, e.e1, f);
if (e.e1.op == EXP.error)
return setError();
Comment on lines -6895 to -6896
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (f.type.ty == Tfunction)
{
Expand Down Expand Up @@ -7207,7 +7269,8 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
}
if (f.needThis())
{
if (hasThis(sc))
auto memberFunc = hasThis(sc);
if (memberFunc && haveSameThis(memberFunc, f))
{
/* Should probably supply 'this' after overload resolution,
* not before.
Expand Down
10 changes: 10 additions & 0 deletions compiler/test/compilable/test19295.d
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
struct S1(T...) {
auto fun() {
static assert(__traits(compiles, &T[0]));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@RazvanN7 RazvanN7 Mar 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

struct S2 {
void gun() {}
S1!gun overloaded;
}
37 changes: 37 additions & 0 deletions compiler/test/compilable/testcorrectthis.d
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// https://issues.dlang.org/show_bug.cgi?id=10886

struct A
{
@property int foo() { return 0; }
int bar() { return 0; }
}

struct B
{
void bar()
{
alias f = typeof(A.foo); // NG
alias b = typeof(A.bar); // ok
}
}

// https://issues.dlang.org/show_bug.cgi?id=21288

struct XA
{
int p;
}

struct XB
{
XA a() { return XA.init; }
alias a this;
}

struct XC
{
void foo()
{
static assert(XB.p.stringof == "p"); // Error: this for s needs to be type B not type C
}
}
2 changes: 1 addition & 1 deletion compiler/test/fail_compilation/fail61.d
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ TEST_OUTPUT:
fail_compilation/fail61.d(22): Error: no property `B` for type `fail61.A.B`
fail_compilation/fail61.d(23): Error: no property `B` for type `fail61.A.B`
fail_compilation/fail61.d(32): Error: no property `A2` for type `fail61.B2`
fail_compilation/fail61.d(41): Error: `this` for `foo` needs to be type `B3` not type `fail61.C3`
fail_compilation/fail61.d(41): Error: need `this` for `foo` of type `void()`
---
*/

Expand Down
18 changes: 0 additions & 18 deletions compiler/test/fail_compilation/ice19295.d

This file was deleted.

4 changes: 2 additions & 2 deletions compiler/test/fail_compilation/ice9439.d
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
/*
TEST_OUTPUT:
---
fail_compilation/ice9439.d(12): Error: `this` for `foo` needs to be type `Derived` not type `ice9439.Base`
fail_compilation/ice9439.d(12): while evaluating: `static assert((__error).foo())`
fail_compilation/ice9439.d(12): Error: need `this` for `foo` of type `int()`
fail_compilation/ice9439.d(12): while evaluating: `static assert(foo())`
fail_compilation/ice9439.d(19): Error: template instance `ice9439.Base.boo!(foo)` error instantiating
---
*/
Expand Down