Skip to content

Fix issue 6400 - Better interaction between with() and opDispatch (Part 2)#7356

Merged
dlang-bot merged 3 commits intodlang:masterfrom
JinShil:fix_6400
Dec 6, 2017
Merged

Fix issue 6400 - Better interaction between with() and opDispatch (Part 2)#7356
dlang-bot merged 3 commits intodlang:masterfrom
JinShil:fix_6400

Conversation

@JinShil
Copy link
Contributor

@JinShil JinShil commented Nov 22, 2017

Revival of #6474 which is a follow-up to #6439 - Specifically this comment by @ZombineDev

It's very unusual that outer symbols are not visible in inner scopes (even if the symbol is rewritten to opDispatch).

Special thanks to @LemonBoy

cc @WalterBright who approved and pulled the original fix in #6467

@dlang-bot
Copy link
Contributor

dlang-bot commented Nov 22, 2017

Thanks for your pull request, @JinShil!

Bugzilla references

Auto-close Bugzilla Description
6400 opDispatch with WithStatement

Copy link
Member

@PetarKirov PetarKirov left a comment

Choose a reason for hiding this comment

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

Looks good (modulo one typo). I think it makes sense to also add a test like this:

enum int base(string name) = 10 * (name[$-1] - '0');
struct Foo { int opDispatch(string name)() { pragma(msg, "Foo"); return base!name + 1; } }
struct Bar { int opDispatch(string name)() { pragma(msg, "Bar"); return base!name + 2; } }
struct Baz {  }

void main()
{    
    assert(test());
    static assert(test());
}

bool test()
{
    auto foo = new Foo;
    auto bar = new Bar;
    auto baz = new Baz;

    with (foo)
    {
        assert(f1() == 11);
        with (baz) assert(f1() == 11);
        with (bar)
        {
            assert(f2() == 22);
            with (baz) assert(f2() == 22);
            with (foo)
            {
                assert(f3() == 31);
                with (baz) assert(f3() == 31);
                with (bar)
                {
                    assert(f4() == 42);
                    with (baz) assert(f4() == 42);
                    with (baz)
                    {
                        assert(f5() == 52);
                        with (baz) assert(f5() == 52);
                    }
                    with (foo)
                    {
                        assert(f6() == 61);
                        with (baz) assert(f6() == 61);
                    }
                    with (bar)
                    {
                        assert(f7() == 72);
                        with (baz) assert(f7() == 72);
                    }
                }
            }
        }
    }

    return true;
}

Also we should document in the spec the rule that the inner most scope wins, if we haven't already.

with (bar)
{
f1();
f1(); // far's scope, so Bar's opDispatch.
Copy link
Member

Choose a reason for hiding this comment

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

far -> bar

@JinShil
Copy link
Contributor Author

JinShil commented Nov 23, 2017

I think it makes sense to also add a test like this:

Done

Also we should document in the spec the rule that the inner most scope wins, if we haven't already.

Correct me if I'm wrong, but 11.19.4 seems to cover it: https://dlang.org/spec/statement.html#with-statement

return;
}
}
break;
Copy link
Member

Choose a reason for hiding this comment

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

Please explain why this is removed?

Copy link
Contributor Author

@JinShil JinShil Nov 26, 2017

Choose a reason for hiding this comment

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

Please see this comment by @ZombineDev

It's very unusual that outer symbols are not visible in inner scopes (even if the symbol is rewritten to opDispatch).

Consider this case, and see the comment about f2():

class Foo
{
    void opDispatch(string name)() { pragma(msg, "Foo"); }
}
class Bar
{
    void opDispatch(string name)() { pragma(msg, "Bar"); }
}
class Baz
{
}

void main()
{
    auto foo = new Foo;
    auto bar = new Bar;
    auto baz = new Baz;

    with (foo)
    {
        f0();
        with (bar)
        {
            f1();
        }
        with (baz)
        {
            f2();   // Current Implementation:  Error: undefined identifier f2
                    //  `opDispatch` is not implemented in `Baz`, but it is implemented
                    // in `Foo`.  Since we are still in `Foo`'s scope, it should use `Foo`'s
                    // `opDispatch`.
        }
    }
}

@andralex
Copy link
Member

This would need a DIP or at least a spec change and a changelog.

@JinShil
Copy link
Contributor Author

JinShil commented Nov 27, 2017

@andralex This PR is not introducing any language change. However, the spec could probably be clarified.

The following code already compiles and runs today:

import std.stdio;

struct Foo
{
    void f() { writeln("Foo.f"); }
}

struct Bar
{
    void f() { writeln("Bar.f"); }
}

struct Baz
{
    // f() is not implemented
}

void f()
{
    writeln("f");
}

void main()
{
    Foo foo;
    Bar bar;
    Baz baz;

    f();               // prints "f"

    with(foo)
    {
        f();           // prints "Foo.f"

        with(bar)
        {
            f();       // prints "Bar.f"

            with(baz)
            {
                f();   // prints "Bar.f".  `Baz` does not implement `f()` so resolution is
                       // forwarded to `with(bar)`'s scope
            }
        }
        with(baz)
        {
            f();       // prints "Foo.f".  `Baz` does not implement `f()` so resolution is
                       // forwarded to `with(foo)`'s scope
        }
    }
    with(baz)
    {
        f();           // prints "f".  `Baz` does not implement `f()` so resolution is forwarded
                       // to `main`'s scope. `f()` is not implemented in `main`'s scope, so
                       // resolution is subsequently forward to module scope.
    }
}

This PR simply brings opDispatch in compliance with the same scope resolution rules demonstrated in the example above.

I've added a clarification to the spec at dlang/dlang.org#1934 and added a changelog entry to this PR.

@JinShil
Copy link
Contributor Author

JinShil commented Nov 28, 2017

@ZombineDev Any idea what's going on with Jenkins? I haven't made any code changes; just doc changes. This PR was passing. Was there an infrastructure change recently?

@PetarKirov
Copy link
Member

@JinShil I'm not sure right now. I'll try to find some time to investigate later today.

@JinShil
Copy link
Contributor Author

JinShil commented Nov 30, 2017

Any idea what's going on with Jenkins?

This regression is causing the Jenkins failure. It should pass once a fix is submitted.

@WalterBright and @andralex Despite the Jenkins failure, this PR is ready for your further consideration. See my comments above addressing your concerns. A changelog entry has been added to this PR, and a spec clarifiation submitted.

Copy link
Member

@andralex andralex left a comment

Choose a reason for hiding this comment

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

OK, I'll green light this. Please fix the numeric reference. Thanks.

@@ -0,0 +1,40 @@
`opDispatch` resolution in `with` statements

`opDispatch` resolution in `with` statements has been modified to bring it into compliance with the existing scope resolution rules in the language specification. See $(LINK2 $(ROOT_DIR)spec/statement.html#with-statement, WithStatement) in the language specification; specifically 11.19.6.
Copy link
Member

Choose a reason for hiding this comment

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

Until we come up with a good system, please refrain from using hardcoded numeric chapter and verse.

@dlang-bot dlang-bot merged commit c5defc8 into dlang:master Dec 6, 2017
@JinShil JinShil deleted the fix_6400 branch December 6, 2017 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants