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

Alternative fix to issue 17548 - [REG2.072.0] Forward reference error with scope function parameters #6937

Merged
merged 2 commits into from Jul 9, 2017

Conversation

Syniurge
Copy link
Contributor

@Syniurge Syniurge commented Jun 25, 2017

Previous PR: #6934

This one line fix ensures that if a StructDeclaration.semantic() call has to defer itself, semanticRun is set to PASSsemantic, because it may already have been set to PASSsemanticdone if a second StructDeclaration.semantic() call on the same struct happened while semantic'ing its members (see how in the Bugzilla issue) and then the deferred call would immediately return.

@dlang-bot
Copy link
Contributor

dlang-bot commented Jun 25, 2017

Thanks for your pull request, @Syniurge! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close Bugzilla Description
17548 [REG2.072.0] Forward reference error with scope function parameters

@Syniurge
Copy link
Contributor Author

I have a question about this code in StructDeclaration.semantic:

    else if (symtab && !scx)
    {
        semanticRun = PASSsemanticdone;
        return;
    }

What's the rationale? Is it assuming that this may only happen if there was a previous StructDeclaration.semantic call which failed with:

        if (!determineFields())
        {
            assert(type.ty == Terror);
            sc2.pop();
            return;
        }

which is why it sets semanticRun to PASSsemanticdone?

If yes the assumption's wrong, and a proper fix would probably be to get rid of that assignment?

@Syniurge Syniurge changed the title Alternative fix to issue 17548 - [REG2.072.0] Forward reference error with scope function parameters [Do not merge yet] Alternative fix to issue 17548 - [REG2.072.0] Forward reference error with scope function parameters Jun 30, 2017
@Syniurge Syniurge changed the title [Do not merge yet] Alternative fix to issue 17548 - [REG2.072.0] Forward reference error with scope function parameters [REG2.072.0] Forward reference error with scope function parameters Jun 30, 2017
Syniurge and others added 2 commits June 30, 2017 20:41
…ction parameters

Prior to this commit, in StructDeclaration.semantic the only two ways the condition (symtab && !scx && semanticRun < PASSsemanticdone) may be true are:
 - if determineFields() fails
 - if a first semantic() call on the same struct is ongoing

But in the second case, setting semanticRun to PASSsemanticdone is wrong, because if the semantic() call defers itself, the deferred semantic() call will return immediately, resulting in a "has forward references" error during semantic2().
@Syniurge
Copy link
Contributor Author

Syniurge commented Jul 1, 2017

So this works too:

         else if (symtab && !scx)
 -        {
 -            semanticRun = PASSsemanticdone;
              return;
 -        }

         (...)

         if (!determineFields())
          {
              assert(type.ty == Terror);
              sc2.pop();
 +            semanticRun = PASSsemanticdone;
              return;
          }

and I think this is the proper fix.

Why isn't the auto-tester running though?

@CyberShadow
Copy link
Member

@braddr Here's another instance of the autotester apparently ignoring a PR.

@Syniurge Syniurge changed the title [REG2.072.0] Forward reference error with scope function parameters Alternative fix to issue 17548 - [REG2.072.0] Forward reference error with scope function parameters[REG2.072.0] Forward reference error with scope function parameters Jul 1, 2017
@Syniurge Syniurge changed the title Alternative fix to issue 17548 - [REG2.072.0] Forward reference error with scope function parameters[REG2.072.0] Forward reference error with scope function parameters Alternative fix to issue 17548 - [REG2.072.0] Forward reference error with scope function parameters Jul 1, 2017
@braddr
Copy link
Member

braddr commented Jul 1, 2017

In this case, it's easy.. new user, not approved yet for builds to be processed. Just look here:
https://auto-tester.puremagic.com/pulls.ghtml?projectid=14

@CyberShadow
Copy link
Member

CyberShadow commented Jul 1, 2017

Aah, forgot about that, thanks!

Edit: Approved @Syniurge.

@UplinkCoder
Copy link
Member

@WalterBright this looks okay at a glance.

@Syniurge
Copy link
Contributor Author

Syniurge commented Jul 5, 2017

Ping?

@CyberShadow CyberShadow requested a review from yebblies July 5, 2017 22:08
@@ -323,10 +323,8 @@ extern (C++) class StructDeclaration : AggregateDeclaration
userAttribDecl = sc.userAttribDecl;
}
else if (symtab && !scx)
{
semanticRun = PASSsemanticdone;
Copy link
Member

Choose a reason for hiding this comment

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

Looks good, so the problem was that we set this to done here, even though we might be in a recursive call to StructDeclaration.semantic, with the outer semantic still ongoing on it's members, hence providing wrong semanticRun state info for any further members.

@dlang-bot dlang-bot merged commit 2f6db2f into dlang:stable Jul 9, 2017
tramker pushed a commit to tramker/dmd that referenced this pull request Feb 8, 2018
Alternative fix to issue 17548 - [REG2.072.0] Forward reference error with scope function parameters
merged-on-behalf-of: Martin Nowak <code@dawg.eu>

cherry-picked-by: Martin Krejcirik <mk@krej.cz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants