[Semantic3 Refactor] setupScope

[thewilsonator]

Part 2 of approx. 23

follows on from #8609

[dlang-bot]

Thanks for your pull request and interest in making D better, @thewilsonator! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
• 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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#8668"

[jacob-carlborg]

Is this required for later usage?

[thewilsonator]

Yes, the Idea it to fold most of the visit into the struct so that all the used local variables become members of the struct. i.e. this will be a common theme.

[WalterBright]

Part 2 of approx. 23

I don't see where this is going. This looks like the brink of a major refactoring - but I don't know its intention. I don't want to waste your time with this unless we can agree on what is happening.

[thewilsonator]

It is, the Idea it to fold most of the visit(FuncDeclation) into the struct with each portion being a single method so that one can look at the overall intention of the function in <3 pages of code:

        if (funcdecl.fbody || funcdecl.frequires || needEnsure)
        {
            /* Symbol table into which we place parameters and nested functions,
             * solely to diagnose name collisions.
             */
            funcdecl.localsymtab = new DsymbolTable();

            sc2 = fds.setupScope();

            fds.nestedFuncLit();

            // Declare 'this'
            auto ad = funcdecl.isThis();
            funcdecl.vthis = funcdecl.declareThis(sc2, ad);
            //printf("[%s] ad = %p vthis = %p\n", loc.toChars(), ad, vthis);
            //if (vthis) printf("\tvthis.type = %s\n", vthis.type.toChars());

            _arguments = fds.declareVarargs();

            fds.declareArgs();
            fds.declareTuples();

            fds.invariants();

            if (funcdecl.fbody)
            {
                sc2 = fds.newScope(sc2);
                auto ad2 = fds.ctorFlowFieldInit();
                fds.inferReturnRef();
                
                if (funcdecl.fbody.isErrorStatement())
                {
                }
                else if (funcdecl.isStaticCtorDeclaration())
                {
                    /* It's a static constructor. Ensure that all
                     * ctor consts were initialized.
                     */
                    ScopeDsymbol pd = funcdecl.toParent().isScopeDsymbol();
                    foreach (s; *pd.members)
                        s.checkCtorConstInit();
                }
                else if (ad2 && funcdecl.isCtorDeclaration())
                {
                    fds.ctorDecl();
                }
                
                /* note about  https://issues.dlang.org/show_bug.cgi?id=17502
                 * removed for brevity
                 */
                funcdecl.buildEnsureRequire();
                
                const blockexit = fds.checkNothow();
                fds/addReturnStatements(blockexit);
                if (funcdecl.returns)
                    fds.checkReturnStatements();

                fds.runNVRO();

                sc2 = sc2.pop();
            }

            fds.requireEnsure();

            if (!(funcdecl.fbody && funcdecl.fbody.isErrorStatement()))
            {
                auto a = new Statements();
                if(!fds.mergeOutInit(a))
                    return;
                fds.setUpVarArgs(a);
                fds.megreContractsWithBody(a);

                if (addReturn0())
                {
                    // Add a return 0; statement
                    Statement s = new ReturnStatement(Loc.initial, new IntegerExp(0));
                    a.push(s);
                }

                Statement sbody = new CompoundStatement(Loc.initial, a);
                fds.appendDtorCalls(sbody);

                // from this point on all possible 'throwers' are checked
                funcdecl.flags &= ~FUNCFLAG.nothrowInprocess;
                if (funcdecl.isSynchronized())
                {
                    fds.synchronize(sbody);
                }

                // If declaration has no body, don't set sbody to prevent incorrect codegen.
                if (funcdecl.fbody || funcdecl.allowsContractWithoutBody())
                    funcdecl.fbody = sbody;
            }

            fds.fixupGotos();

            if (funcdecl.naked && (funcdecl.fensures || funcdecl.frequires))
                funcdecl.error("naked assembly functions with contracts are not supported");
            
            sc2.ctorflow.callSuper = CSX.none;
            sc2.pop();
        }

        if (funcdecl.checkClosure())
        {
            // We should be setting errors here instead of relying on the global error count.
            //errors = true;
        }

        fds.inferAttributes();
        fds.inferScope();

rather than ~1100 lines in a single function with little to no comments.

This is required for me to even being contemplating trying to fix issue 14246 in a way that breaks as little as possible. I can't understand an 1100 line function, and I wouldn't expect anyone else to either.

[RazvanN7]

I suggest the name createScope as that is what it actually does.

[Geod24]

Since 2 members approved this, and no opposition have been voiced for a week, I took the liberty to add the 72h no objection -> merge label.

[WalterBright]

I object to it. In particular, I object to the establishment of struct FuncDeclSem3 which was merged earlier that I object to, and this PR continues to add to it. Structs should not be used for aggregations of random functions.

[thewilsonator]

As stated earlier this is not aggregation of random functions, this is to make semantic3.visit(FuncDeclaration) actually comprehendible. No, the Scope does not hold all the variables used, there are, in the final refactored version:

// Scopes:
// Scope of analysis
Scope* sc;
// Scope created to avoid name collisions
Scope* sc2;
// Scope for out contract return variable
Scope* scout;

// The FuncDeclaration subject to Semantic analysis
FuncDeclaration funcdecl;
TypeFunction f;  //funcdecl.type
// Does funcdecl need a closure?
bool needEnsure;

// The variable arguemnt ... parameter.
VarDeclaration _arguments;

// Precondition and postcondition invariants
Statement fpreinv, fpostinv;

// In / Out contract statements
Statement freq, fens;

// funcdecl.isThis, funcdecl.isMember2
AggregateDeclaration ad, ad2;

All or which are used all over the place. Sure, the declarations of the above could be littered throughout the function, but they are better served, documented, in one place, away from all the other variables declared that aren't used further than the construction of one that is (which is what this struct is for).

These PRs are blocking a fix for issue 14246, which is a critical bug for Weka (it's even pre approved!), somewhat ironically I think this quote is an apt description of my feelings at the moment.

[WalterBright]

They are better served and documented as parameters to the function(s). That way, it is clear what the interface to the function is.

[WalterBright]

#6816 is the solution to https://issues.dlang.org/show_bug.cgi?id=14246, and does not require any refactoring. The trouble with 6816 is dealing with the breakage of existing code that is incorrectly written (such as running unsafe destructors in @safe constructors). I plan on dealing with this using a compiler transition switch.

[thewilsonator]

#6816 is the solution to https://issues.dlang.org/show_bug.cgi?id=14246, and does not require any refactoring.

Maybe because you know the compiler like the back of your hand and can comprehend an 1100 line function. I don't/can't, hence the refactor. I also note the the above PR resulted in large code breakage, and therefore a proper fix requires more finesse.

The trouble with 6816 is dealing with the breakage of existing code that is incorrectly written (such as running unsafe destructors in @safe constructors).

IIUC, the main problem was running destructors on objects that were not constructed (yes destructors should able to handle a non-constructed object, but such is reality. It will stay like that until (if at all) the compiler inserts units to do so).

The correct solution, modulo problems with safe, is to do the really expensive/extensive checking of initialisation of fields that have destructors, noting that constructors that are nothrow (or are inferred nothrow) do not require the checks since they cannot throw so that perf does not degrade (this also solves the @safe problem), and probably adding -vtrhowingctor and -vthrowingdtor since we should discourage con/destructors that throw.

This will ensure that
a) the fix breaks no code
b) there is minimal pref loss
c) there is a clear path to the aversion of said perf loss.
d) the @safe problem is limited to constructors that throw and are @safe that have destructors that are @system

[RazvanN7]

@thewilsonator It seems that @WalterBright is against the struct that holds all the variables. I was against it too for the same reason, but I was willing to make a compromise for the sake of comprehensibility, however doing so is inconsistent with the whole dmd codebase. I think that the way to go forward with this is to lose the struct and create documented free functions and from there see how we can reduce the number of parameters (most of the variable declarations in semantic3 for FuncDeclaration are coming from the Scope visitor member or from funcdecl, so you could just pass the same references).

[jacob-carlborg]

I was against it too for the same reason, but I was willing to make a compromise for the sake of comprehensibility, however doing so is inconsistent with the whole dmd codebase.

You mean that the whole DMD code base is incomprehensible? 😃.

[AndrewEdwards]

@thewilsonator Need you to decide if you intend to pursue this further. I'm assuming you do since the PR is still open. If that is the case, please address @WalterBright's concerns so we can move forward.

[AndrewEdwards]

Ping @thewilsonator

[RazvanN7]

Closing as per @WalterBright 's decision