Skip to content

No-op string in constructor is repeated and reordered when constructor assignment is used #48671

Closed
@rhuitl

Description

@rhuitl

Bug Report

The TypeScript code

class Example {
    constructor(private A: string) {
        "ngInject";
    }
}

when compiled with TS 4.6.2 yields:

"use strict";
class Example {
    constructor(A) {
        this.A = A;
        "ngInject";
        "ngInject";
    }
}

when compiled with TS 4.5.5 yields:

"use strict";
class Example {
    constructor(A) {
        "ngInject";
        this.A = A;
    }
}

The differences are:

  • the string "ngInject;" is repeated
  • the string appears after the constructor assignment, not before as in 4.5.

For our use case, the repetition is probably not a real problem, however the re-ordering is. If any of these two are by design now, we can work around - but I wanted to double check with the TypeScript developers because it seems to be a bit unexpected.

🕗 Version & Regression Information

We observed errors in a AngularJS-based project using ng-annotate-patched which relies on the string to annotate classes for AngularJS. It works wiht TS 4.5 and broke with TS 4.6.

TS 4.6.2 (broken)
TS 4.5.5 (good)

⏯ Playground Link

https://www.typescriptlang.org/play?removeComments=true&target=2&ts=4.5.5#code/MYGwhgzhAECiAeYC2AHEBTaBvAUASGAHsA7CAFwCcBXYMwigChQoEsA3MMzAQQC5pyrYgHMAlNnx4ARCICSxAFbpaUgNz4Avji1A

Activity

RyanCavanaugh

RyanCavanaugh commented on Apr 13, 2022

@RyanCavanaugh
Member

👉 @JoshuaKGoldberg if you feel like it

jakebailey

jakebailey commented on Apr 13, 2022

@jakebailey
Member

The repetition is weird, but the reordering doesn't seem to be wrong to me at first glance; I would think that the parameters being properties implies that they must be assigned before any part of the body executes.

Is there some special case for these sort of string-y statements at the start of blocks? Playground Link

I guess so, because "use strict".

JoshuaKGoldberg

JoshuaKGoldberg commented on Apr 13, 2022

@JoshuaKGoldberg
Contributor

Yes, they're directive prologues, also sometimes callied prologue directives. I've only ever seen "use esm" and "use strict" in the wild till now. TIL about ng-annotate-patched.

#29374 and https://blog.joshuakgoldberg.com/code-before-super-technical-overview/ have a good amount of backing context for how those are meant to be at the beginning of the constructor.

I can probably take a look within this week!

Edit: or today 😄

jakebailey

jakebailey commented on Apr 13, 2022

@jakebailey
Member

Also TIL that the spec lets you have whatever prologues you want. 😄

fatcerberus

fatcerberus commented on Apr 13, 2022

@fatcerberus

I mean, why wouldn't it? They're just strings, if the engine doesn't recognize a directive, it can just ignore it as a no-op.

jakebailey

jakebailey commented on Apr 13, 2022

@jakebailey
Member

I just mean from the POV that there's nothing wrong with this code:

function foo() {
    console.log("Hello, world!");
    console.log(Math.random());
    "some random string as an expression statement";
}

Because a string is an expression which can be alone in a statement.

But if it's the first statement of a source file or a function body, then it's special and must not be reordered even if our emit is going to be placing things semantically "before" the block the user typed, because that changes its meaning.

jakebailey

jakebailey commented on Apr 13, 2022

@jakebailey
Member

FWIW this bug applies with "use strict" too (even though for some reason, running this emitted code crashes; running it elsewhere does not).

Playground Link

https://glot.io/snippets/g8u41v3925

fatcerberus

fatcerberus commented on Apr 13, 2022

@fatcerberus

Out of curiosity, does the spec say anything about multiple directives? i.e. if I write...

function foo() {
    "use fooey";
    "use bard";
    console.log(812);
}

...are both strings required to be kept at the top of the function?

jakebailey

jakebailey commented on Apr 13, 2022

@jakebailey
Member

I don't know about the spec, but at runtime it sure seems like it applies all of them: https://glot.io/snippets/g8u48lkc4o (don't ask me what runtime this site uses)

JoshuaKGoldberg

JoshuaKGoldberg commented on Apr 13, 2022

@JoshuaKGoldberg
Contributor

From https://es5.github.io/#x14.1:

A Directive Prologue may contain more than one Use Strict Directive. However, an implementation may issue a warning if this occurs.

9 remaining items

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

BugA bug in TypeScriptFix AvailableA PR has been opened for this issue

Type

No type

Projects

No projects

Relationships

None yet

    Development

    Participants

    @rhuitl@DanielRosenwasser@JoshuaKGoldberg@fatcerberus@jakebailey

    Issue actions

      No-op string in constructor is repeated and reordered when constructor assignment is used · Issue #48671 · microsoft/TypeScript