Skip to content

Misplaced or partially removed comments in emitted code #32813

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

Closed
falsyvalues opened this issue Aug 12, 2019 · 6 comments
Closed

Misplaced or partially removed comments in emitted code #32813

falsyvalues opened this issue Aug 12, 2019 · 6 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@falsyvalues
Copy link

TypeScript Version: 3.5.1

Search Terms:
comments comment mangling emitted code doc docs strip code

Code
Especially annoying problem while using tools like strip code etc.

class ABC {
    // #DEBUG
    private a () {}
    // #ENDDEBUG
    public z = 1;
}

Expected behavior:

"use strict";
class ABC {
    constructor() {
        this.z = 1;
    }
    // #DEBUG
    a() { }
    // #ENDDEBUG
}

Actual behavior:

"use strict";
class ABC {
    constructor() {
        // #ENDDEBUG
        this.z = 1;
    }
    // #DEBUG
    a() { }
}

Playground Link:
Misplaced:
http://www.typescriptlang.org/play/#code/MYGwhgzhAECCBCBhaBvAUNT0D03oGIARAUXgFUBxDLABwCcBLANzABcBTaMaACgEpUAX2qZcBYgDlCJclSzQaAVwBGIBsGgAvaAF5oARgDcaQUA
Removed:
http://www.typescriptlang.org/play/#code/MYGwhgzhAECCBCBhaBvAUNT0D03oGIARAUXgFUBxDLABwCcBLANzABcBTaMaACgEpUAX2qZcBYgDlCJclUFA

Related Issues:
Checked #1311 and #10385. The #1311 works better but still failing in one case.

@nmain
Copy link

nmain commented Aug 12, 2019

I see your point, but without understanding the content of the comment it's hard for typescript to satisfy everyone. The following example is identical to yours except for the content of the comments but probably matches what the developer wanted:

Playground

@falsyvalues
Copy link
Author

Hi @nmain! In this particular case, removed comments were required for strip code applied in next build steps and this step was failing because of missing comments (we do parity check to ensure correctness of removal).

@nmain
Copy link

nmain commented Aug 12, 2019

Sure, but how do you make the compiler succeed on your case and not fail on mine? // #ENDDEBUG looks just as much like leading trivia for the next declaration as it does trailing trivia for the previous one.

@falsyvalues
Copy link
Author

I'm not saying that trivial problem, but losing comments is the least expected result.

@nmain
Copy link

nmain commented Aug 12, 2019

Agreed. I was focusing more on your first case, but the removed comment should certainly not be removed.

@RyanCavanaugh RyanCavanaugh added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Aug 16, 2019
@RyanCavanaugh
Copy link
Member

As noted, the "expected" ordering is entirely up to reading the semantics of the comment and guessing and what it means. That isn't something TS can do anything about.

Comments are always attached to the following non-comment thing, and if that thing is a token (like a closing brace) instead of a node (like a declaration), then the comment isn't stored anywhere and thus can't be emitted. Some comments are indeed going to be lost; this is a trade-off for performance and simplicity we've decided to adopt.

Overall we're not going to invest more of the limited complexity and performance budget into making comment emit better than it already is - if something works, great, but comments aren't semantics and we can only realistically made semantic guarantees about JS output.

As comparison, this is Babel's output:

function () {
  function ABC() {
    _classCallCheck(this, ABC);

    this.z = 1;
  } // #DEBUG


  _createClass(ABC, [{
    key: "a",
    value: function a() {} // #ENDDEBUG

  }]);

  return ABC;
}();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

3 participants