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 right this check #1687

Merged
merged 10 commits into from Mar 1, 2013
Merged

Fix right this check #1687

merged 10 commits into from Mar 1, 2013

Conversation

9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Feb 24, 2013

(This change is based on #1406)

Currently, dmd front-end does not detect incorrect field access that has no valid 'this' context. Such invalid expression will be detected in glue layer later.

struct S { int val; }
void main() { int n = S.val; }  // !

This change fixes the issue. The call of resolveProperties is good place to check it.

Some "symbol/type related" expressions, e.g. S.val.offsetof, S.val.init, are still valid after this change.

A symbol access through "alias this" is treated specially. If Type.member is implicitly translated to Type.aliasthis.member, it is further translated to typeof(Type.aliasthis).member.


This change additionally fixes:
Issue 1680 - static struct constructor overloaded with method prevents compilation in inner function

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 24, 2013

Requires:
dlang/druntime#425
dlang/phobos#1158

@ghost
Copy link

ghost commented Feb 24, 2013

Requires:
dlang/druntime#425
dlang/phobos#1158

It seems like this pull is going to break code.

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 24, 2013

It seems like this pull is going to break code.

Yes, very slightly code will be broken. Because they were invalid but accidentally had been accepted.

@ghost
Copy link

ghost commented Feb 24, 2013

Yes, very slightly code will be broken. Because they were invalid but accidentally had been accepted.

This is a perfect time to introduce a file with a list of language changes for the upcoming version, so we're reminded to add this language change to the changelog before release.

@@ -764,6 +764,7 @@ elem *setArray(elem *eptr, elem *edim, Type *tb, elem *evalue, IRState *irs, int

elem *Expression::toElem(IRState *irs)
{
printf("[%s] %s ", loc.toChars(), Token::toChars(op));
Copy link
Member

Choose a reason for hiding this comment

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

Debug printf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We never come here in normal, because dmd will assert(0) after that immediately. This is useful in order to know quickly the place where is actually broken.

@yebblies
Copy link
Member

Is there any chance you can move both the checking and the resolution out of e2ir? Otherwise getrightthis & friend get duplicated for every backend.

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 24, 2013

@yebblies That would be a challenge for the future. We should to advance it one step by step.

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 24, 2013

This is a perfect time to introduce a file with a list of language changes for the upcoming version, so we're reminded to add this language change to the changelog before release.

@AndrejMitrovic I agree to do it.

@ghost
Copy link

ghost commented Feb 24, 2013

Shouldn't testrightthis.d really be in compilable? From what I can tell the module does no runtime checks at all and doesn't belong in runnable. OTOH this seems to be decided completely arbitrary so maybe it doesn't matter..

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 24, 2013

@AndrejMitrovic In the future, we need to remove all 'right this' check from glue layer. To do it, adding tests for code generation is necessary. test/runnable/testrightthis.d is good place for that.

Dsymbol *p = sc->parent;
while (p && p->isTemplateMixin())
p = p->parent;
FuncDeclaration *fdthis = p ? p->isFuncDeclaration();
Copy link
Member

Choose a reason for hiding this comment

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

Oh, now it's still missing the : NULL else part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, no! Thanks very much for your mention. Fixed again.

@MartinNowak
Copy link
Member

I'm not really positive about the implicit typeof and the error when calling static functions on member variables.
Shouldn't we preserve the current behavior and only check for 'this' when it actually gets used?

struct Foo
{
    static struct Bar
    {
        static int get() { return 0; }
    }
    Bar bar;
    alias bar this;
}

void test8169()
{
    // allowed cases that do 'use' Foo.bar without this
    static assert(Foo.get() == 0);             // typeof(Foo.bar).get()
    static assert(typeof(Foo.bar).get() == 0); // typeof(Foo.bar).get()
    static assert(Foo.bar.offsetof == 0);      // Foo.bar.offsetof
    // this is now an error, but it used to work
    static assert(Foo.bar.get() == 0);         // Foo.bar.get()
}

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 26, 2013

@dawgfoto Hmm, to keep current behavior is not impossible. It requires a little more work in CallExp and DotVarExp.
I’ll consider it.

@9rnsr 9rnsr closed this Feb 26, 2013
@9rnsr 9rnsr reopened this Feb 27, 2013
@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 27, 2013

Updated for @dawgfoto 's review point.
Now, the unreal variable access doesn't make errors if it is not actually used.

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 27, 2013

And, recent Phobos change had shown the incompleteness of my change. To fix the problem, I added one more commit.

… inside member function

Use ThisExp instead of VarExp to avoid interpretation on template arguments.
And, distinguish typeof(Type.var) and typeof({ auto x = Type.var; }) by sc->intypeof == 1 or 2.
Additional fix for Issue 3775 - Segfault(cast.c): casting no-parameter template function using property syntax

Allow using Type.templatefunc as a property
…iasthis).member

And, add some test cases for 'alias this'.
Add test cases for accessing 'this' from constraint and DeclDefs scope
WalterBright added a commit that referenced this pull request Mar 1, 2013
@CyberShadow
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants