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

Suggestion: disallow use before definition #21

Closed
RyanCavanaugh opened this issue Jul 15, 2014 · 29 comments
Closed

Suggestion: disallow use before definition #21

RyanCavanaugh opened this issue Jul 15, 2014 · 29 comments
Labels

Comments

@RyanCavanaugh
Copy link
Member

@RyanCavanaugh RyanCavanaugh commented Jul 15, 2014

The compiler should issue an error when code uses values before they could possibly be initialized.

// Error, 'Derived' declaration must be after 'Base'
class Derived extends Base { }
class Base { }
@everson
Copy link

@everson everson commented Jul 21, 2014

While throwing a compiler error is a good solution, perhaps the compiler could output the classes in the right order. That would be a killer feature. E.g. The compiler keeps track of dependency relationship and output the classes according to that, throwing the compiler error only when unable to resolve the dependency order.

@jvilk
Copy link

@jvilk jvilk commented Jul 21, 2014

The compiler keeps track of dependency relationship and output the classes according to that, throwing the compiler error only when unable to resolve the dependency order.

Should we make this a new suggestion? This is why I currently use AMD modules rather than TypeScript internal modules; the RequireJS compiler determines the appropriate module serialization order using the dependencies I specify across the codebase (using require()).

@RyanCavanaugh
Copy link
Member Author

@RyanCavanaugh RyanCavanaugh commented Jul 28, 2014

Linking to #274. We need to outline what the rules and scope of this would be

@mhegazy
Copy link
Contributor

@mhegazy mhegazy commented Oct 24, 2014

The extends case seems like a good candidate for lexical checking; we just need to ensure that lexically the base class comes before the derived one. Are there other cases that we should consider?

@sparecycles
Copy link
Contributor

@sparecycles sparecycles commented Oct 24, 2014

One issue is that reordering class definitions might reorder the static initializer order silently. If the base class comes after a derived class I vote to either maintain the static initializer code at the class definition site or just flag an error.

I think that the multiple file case is more interesting and useful from a large project / maintenance standpoint (which is the ostensible goal of typescript, after all).

So, I think we need to consider the output order in single-file output mode. (it would also be nice to be able to get this order for building html files which include multiple files).

Here are some statements that I think would ensure ordering:

class X extends Y {} // ensure Y is defined in prior file
module { new X(); } // ensure X is defined in prior file
class S { static z = new Z(); } // ensure Z is defined in prior file

We could also extend this to functions and variables being defined before use, not just classes.

P.S. I have a prototype.

@danquirk
Copy link
Member

@danquirk danquirk commented Oct 24, 2014

I don't think there is any intention to attempt to reorder emit for you, only to give errors where we can for things that are sure to fail at runtime.

@sparecycles
Copy link
Contributor

@sparecycles sparecycles commented Oct 24, 2014

Dan, I agree with you about reordering within a single file, but when multiple files are combined using --out, the compiler has control over the emit order and I'd prefer that the order it chooses, works.

@mhegazy
Copy link
Contributor

@mhegazy mhegazy commented Oct 24, 2014

@sparecycles functions are hoisted to the top of the scope anyways at run time. so functions are not interesting. variables are hoisted as well, the issue is that they will not be initialized at that point. now use before initialization is a different issue, and I think it is tracked under #274.

for reordering; the philosophy we have followed, is to let the output code be as close as possible to the input code. in essence we let the user code pass through, we just strip out types. in this sense, an error would be more inline with what we have done thus far.

As for the implementation, I have added a lexical order verification recently with Let and Const, and can be extracted out as a general check and used for these different cases. you can find it here:
https://github.com/Microsoft/TypeScript/blob/master/src/compiler/checker.ts#L329

We need to clearly identify the cases where we are checking, and a PR would be definitely welcomed :)

@sparecycles
Copy link
Contributor

@sparecycles sparecycles commented Oct 24, 2014

Yes, I agree that we don't want to reorder within a single typescript file, but in the --out file case, the order is not specified by the user so, again, I would prefer that the compiler makes a best effort to choose an order that works.

The function hoisting is a good example of where we don't need to care in the single file case, but where compiling to multiple files and choosing a sequence to include them in an .html file can be non-trivial for a human. Variables being undefined at use is a great example of where unexpected behavior can be introduced by the compiler because of a change in /// <reference> lines.

@RyanCavanaugh
Copy link
Member Author

@RyanCavanaugh RyanCavanaugh commented Oct 24, 2014

but in the --out file case, the order is not specified by the user

This isn't really the case. We have very simple rules here -- use the order implied by the reference tags and the order of files on the command-line. In both cases, the user is providing us an order. Having the compiler ignore the ordering that the user provided is a dangerous route to go down. What if the compiler decides an order different from the one you'd prefer? How would you override that? What if one order breaks 2 classes and another order breaks 2 variables?

@sparecycles
Copy link
Contributor

@sparecycles sparecycles commented Oct 24, 2014

Then we shouldn't change the order but should we at least (have the option to) warn the user that the order the compiler uses is probably wrong?

@mhegazy
Copy link
Contributor

@mhegazy mhegazy commented Oct 25, 2014

yup. we should not order, but error instead.

@mhegazy mhegazy added Bug and removed Bug labels Dec 9, 2014
@mhegazy mhegazy added this to the TypeScript 1.5 milestone Dec 9, 2014
@mhegazy mhegazy removed this from the TypeScript 1.5 milestone Dec 11, 2014
@mhegazy mhegazy added this to the TypeScript 1.6 milestone Dec 11, 2014
@mhegazy mhegazy added this to the TypeScript 1.6 milestone Dec 11, 2014
@mhegazy mhegazy removed this from the TypeScript 1.5 milestone Dec 11, 2014
@metaweta
Copy link

@metaweta metaweta commented Dec 13, 2014

What's the right idiom in TypeScript for mutually recursive classes? A declare class before the actual definition?

@RyanCavanaugh
Copy link
Member Author

@RyanCavanaugh RyanCavanaugh commented Dec 17, 2014

If your classes simply refer to each other in the type system or in instance methods, that's no problem. The only "mutually recursive" pattern that's a problem is this one:

class Alpha {
    static myFriendBeta = new Beta();   
}

class Beta {
    static myFriendAlpha = new Alpha(); 
}

You can rewrite this as a clodule:

class Alpha {
}

class Beta {
    static myFriendAlpha = new Alpha();
}

module Alpha {
    export var myFriendBeta = new Beta();
}

@sparecycles
Copy link
Contributor

@sparecycles sparecycles commented Feb 1, 2015

When the base class and derived classes are in the same file, the problem is not as bad: the program will crash on startup and the programmer will change the order in that one file and the problem will be fixed.

The multiple file case is where the compiler should warn when concatenating multiple files in a dubious order, because that order can change for subtle reasons.

Consider the case where there is a base class and multiple derived classes, all in separate files. The base class uses some of the derived classes in its implementation, and so it references them, but it still needs to be placed first in the output. Similarly, all the derived classes need to reference the base class.

Well, there is no problem with mutually referenced files, if A.ts mutually references B.ts and X.ts includes A.ts, then the output order will be [B,A,X], and if it references B.ts the order will be [A,B,X]. (But only one of these orders might work at runtime.) This makes things fragile, as compilation will succeed equally well if either B or A is referenced.

My solution for my base-derived class system problem: add an index.ts for my class hierarchy and include, in this file, all the derived classes, followed by the base class. This guaranteed that the output would put the base class first. (completely counter-intuitive!). I found that if I directly referenced the files I wanted it would end up generating the base class after the derived one.

The compiler warning will be very nice, but it would also be great to be able to flag one of the references in a mutual-reference scenario as emit-ordering, and the other is just for pulling in declarations. Mutual emit-ordering references would be an error.

( I currently have this implemented for automatically generating the list of .js includes in our Visual Studio/Typescript project since we're using multi-file output (easier to debug). But the code is in C# as an inline task. If there is interest I will ask if I can share it. It's basically two runs of Tarjan's CC algorithm. )

Both warning when emit-order is wrong and stabilizing emit-order with an explicit directive would go a long way toward making typescript a viable language for large projects... yes?

@xwipeoutx
Copy link

@xwipeoutx xwipeoutx commented Feb 16, 2015

I run into this issue fairly frequently with my rather small code base (around 80 .ts files). Ideally, I'd like to not have any <reference> tags at the top of any of my files, and the compiler can work it all out for me.

My app has only 1 file that instantiates classes and executes the application (my composition root), a few files that add extensions (eg. adding Array.prototype.distinct) and the rest are just class/interface definitions.

In this case, most of the code is fair game for reordering, and shouldn't require manually <reference> definitions to get right. I see class definitions as being fair game for any compiler reordering, and should be shunted up to the top of the combined output, while the rest of the statements can preserve ordering as it was when input.

Would a compiler flag --looseSorting be possible? It seems like a fairly sought after feature.

@yuit
Copy link
Contributor

@yuit yuit commented Mar 17, 2015

In emit class-declaration in ES6, we do static property assignment after class-declaration. This makes referring to class static property in computed property name becomes use-before-definition.

Emitted JS:

class C {
    [C.p] () {}  // Use before definition
    [C.p+ C.e]() {}  // Use before definition
    [D.f] () {}  // Use before definition
}
C.p = 10;
C.e = 20;

class D {
}
D.f = "hi";

We only want to warn about this error in two cases: computed-property names refers to its class static property, or refer to other class, that defined below it, property.

@DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Mar 18, 2015

The trivial example we were playing with today, just to include in any tests:

function f() {
    function g() {
        i = 10;
    }

    let i = 20;
    g();
}

It'd be good to get the permutations of uses/definitions of g around i.

@jvilk
Copy link

@jvilk jvilk commented Mar 18, 2015

Don't forget to think about functions defined within block scope. That's undefined behavior as per the JavaScript standard, and I know at least Firefox and Chrome disagree in their implementation.

e.g.:

function f() {
    if (true) {
        g(); // iirc, g executes in Chrome, and is undefined in Firefox
        function g() {
        }
        g(); // works in both browsers
    }
}

@danquirk
Copy link
Member

@danquirk danquirk commented Apr 21, 2015

Tracked by #2854 now.

@BrainCrumbz
Copy link

@BrainCrumbz BrainCrumbz commented May 4, 2016

Just to mention that we've just being bitten by this today, and it took us some time to figure out what was going on.

TypeScript v1.8.10, Webpack-based build, both base and derived class defined in same file, but (apparently) in the wrong order, no compile errors nor warnings, and even if source maps are working, the error call stack was pointing to a highly unuseful location (the end of another class importing the derived one).

Not going through the whole discussion, but as a first aid it seems like a compiler warning would help. Just our 2¢

@MrGuardian
Copy link

@MrGuardian MrGuardian commented Oct 11, 2016

I find it ridiculous that TS does not support this feature out of the box. The confusion it causes is similar as using standard JS. Also, virtual methods, anyone?

@RyanCavanaugh
Copy link
Member Author

@RyanCavanaugh RyanCavanaugh commented Oct 11, 2016

@MrGuardian the repro described in the OP has been fixed. Perhaps you can clarify in a new issue or existing issue that better describes the problem you're having?

@Spongman
Copy link

@Spongman Spongman commented Dec 2, 2016

(#12673) here's another two cases that IMO should be errors:

class Test
{
    _b = this._a; // undefined, no error/warning
    _a = 3;

    static _B = Test._A; // undefined, no error/warning
    static _A = 3;
    
    method()
    {
        let a = b; // Block-scoped variable 'b' used before its declaration
        let b = 3;
    }
}

@RyanCavanaugh
Copy link
Member Author

@RyanCavanaugh RyanCavanaugh commented Dec 5, 2016

@Spongman can you log that in a separate issue please? Thanks!

@Spongman
Copy link

@Spongman Spongman commented Dec 5, 2016

#12673

@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet