Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Issue 22136 - [REG 2.097.1] hashOf failed to compile because of d… #13404

Merged
merged 1 commit into from
Dec 21, 2021

Conversation

edi33416
Copy link
Contributor

@edi33416 edi33416 commented Dec 8, 2021

…ifferent inheritance order

druntime's hashOf uses the following "idiom"(?) (extracted only the relavant parts)

size_t hashOf(T)(T val)
{
    static if (is(__traits(parent, val.toHash) P) && !is(immutable T* : immutable P*))
        return val ? hashOf(__traits(getMember, val, __traits(getAliasThis, T))) : 0;
    [...]
}

It seems that it's assuming that if you can access toHash through a parent P, and T* is not implicitly convertible to P*
then you're accessing toHash through an alias this. I'm unsure if this is sound, as the class spec says at point 15.16.2: "A class or struct can be implicitly converted to the AliasThis member.".
Either I'm misinterpreting the spec, or the idiom is wrong.

Assuming the idiom is right, the is(immutable T* : immutable P*) check fails for the following example

interface IObject
{
    size_t toHash() @trusted nothrow;
}

interface Dummy {}
interface Bug(E) : Dummy, IObject {}
interface OK(E) : IObject, Dummy {}

void main()
{

    {
        Bug!string s;
        size_t t = hashOf(s);
    }
    {
        OK!string s;
        size_t t = hashOf(s);
    }

    static assert(is(immutable Bug!string* : immutable IObject*)); // <-- this errors with master
    static assert(is(immutable OK!string* : immutable IObject*));
}

As you can see, the order of the interfaces affects the implicit conversion rules.
Before those changes, the implicit conversion check only looked at the declaration (ClassDeclaration or InterfaceDeclaration) at offset 0.
I don't understand why we wouldn't want to check against all the interfaces that a type implements.
Am I missing something fundamental?

@dlang-bot
Copy link
Contributor

dlang-bot commented Dec 8, 2021

Thanks for your pull request and interest in making D better, @edi33416! 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
22136 regression [REG 2.097.1] hashOf failed to compile because of different inheritance order

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

@thewilsonator
Copy link
Contributor

target stable.

@RazvanN7 RazvanN7 added the Severity:Regression PRs that fix regressions label Dec 9, 2021
@edi33416 edi33416 changed the base branch from master to stable December 9, 2021 17:09
@@ -196,7 +196,8 @@ static assert(is( X!( C***, B*** ) == const(B**)* )); // `B***`

static assert(is( X!( C*, I* ) == I* ));
static assert(is( X!( I*, C* ) == I* ));
static assert(Error!( C**, I** ));
//static assert(Error!( C**, I** ));
static assert(is( X!( C**, I** ) == const(I*)* ));
Copy link
Contributor

@RazvanN7 RazvanN7 Dec 10, 2021

Choose a reason for hiding this comment

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

Is this supposed to be const?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unsure about this, but I think so, as this is already true at L313
static assert(is( X!( C*[4], const(B*)[4] ) == const(B*)[] )); // !?

@RazvanN7
Copy link
Contributor

I think you should rebase on top of latest stable to fix the tester issues.

@RazvanN7 RazvanN7 added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Dec 18, 2021
@RazvanN7 RazvanN7 merged commit 646ec17 into dlang:stable Dec 21, 2021
@n8sh
Copy link
Member

n8sh commented Mar 2, 2022

Am I missing something fundamental?

@edi33416 @RazvanN7 Yes! When an interface has multiple parent interfaces one is the "direct parent" that has the same address and the others are at an offset. The following code doesn't compile with DMD 2.098 but in the preview release of DMD 2.099 with this PR it compiles and calls the wrong function:

interface IObject
{
    string getName() const;
}

interface Dummy { string foo(); }
interface Bug : Dummy, IObject { string bar(); }
interface Ok : IObject, Dummy { string baz(); }

class BugImpl : Bug
{
    override string getName() const { return "BugImpl.name"; }
    override string foo() { return "BugImpl.foo"; }
    override string bar() { return "BugImpl.bar"; }
}

class OkImpl : Ok
{
    override string getName() const { return "OkImpl.name"; }
    override string foo() { return "OkImpl.foo"; }
    override string baz() { return "OkImpl.baz"; }
}

void main()
{
    static void printName(const IObject* p)
    {
        import std.stdio;
        writeln(p.getName);
    }

    Bug bug = new BugImpl();
    Ok ok = new OkImpl();

    printName(&bug); // On my machine prints BugImpl.foo instead of BugImpl.name!
    printName(&ok); // Prints OkImpl.name as expected.
}

The specification talks about this at https://dlang.org/spec/abi.html#classes:

The leftmost side of the inheritance graph of the interfaces all share
their vptrs, this is the single inheritance model. Every time the
inheritance graph forks (for multiple inheritance) a new vptr is
created and stored in the class' instance.

n8sh added a commit to n8sh/dmd that referenced this pull request Mar 2, 2022
…use of different inheritance order (dlang#13404)"

This reverts commit 646ec17.
@n8sh
Copy link
Member

n8sh commented Mar 2, 2022

I'll add that I missed that same fundamental thing when writing that code. The error was not in DMD but in the "idiom": it incorrectly regards calling toHash through multiple inheritance the same as calling it through alias this.

I'm unsure if this is sound, as the class spec says at point 15.16.2: "A class or struct can be implicitly converted to the AliasThis member.".

That part isn't a mistake. Consider the difference between is(A : B) and is(A* : B*): an int implicitly converts to a double but a pointer to an int doesn't implicitly convert to a pointer to a double!

n8sh added a commit to n8sh/druntime that referenced this pull request Mar 3, 2022
n8sh added a commit to n8sh/druntime that referenced this pull request Mar 3, 2022
n8sh added a commit to n8sh/druntime that referenced this pull request Mar 3, 2022
n8sh added a commit to n8sh/druntime that referenced this pull request Mar 3, 2022
RazvanN7 pushed a commit that referenced this pull request Mar 3, 2022
…use of different inheritance order (#13404)" (#13745)

This reverts commit 646ec17.
n8sh added a commit to n8sh/druntime that referenced this pull request Mar 8, 2022
n8sh added a commit to n8sh/druntime that referenced this pull request Mar 8, 2022
n8sh added a commit to n8sh/druntime that referenced this pull request Mar 8, 2022
n8sh added a commit to n8sh/druntime that referenced this pull request Mar 8, 2022
@MartinNowak MartinNowak mentioned this pull request Mar 8, 2022
@edi33416
Copy link
Contributor Author

@n8sh Thanks a lot for the clarification. I totally missed that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge:72h no objection -> merge The PR will be merged if there are no objections raised. Severity:Bug Fix Severity:Regression PRs that fix regressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants