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

Jump to bottom

dart2js: --analyze-all breaks code and generally generates different code #17115

Closed
DartBot opened this issue Feb 25, 2014 · 10 comments
Closed

dart2js: --analyze-all breaks code and generally generates different code #17115

DartBot opened this issue Feb 25, 2014 · 10 comments
Assignees
@johnniwinther
Labels
type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) type-enhancement A request for a change that isn't a bug web-dart2js

Comments

@DartBot
Copy link

DartBot commented Feb 25, 2014

This issue was originally filed by cor...@gmail.com

Given the following HTML:

    <!doctype html>
    <html lang="en">
    <head>
        <script type="application/dart" src="main.dart"></script>
        <script type="text/javascript" src="dart.js"></script>
    
    </head>
    <body>
    
    <div class="top-menu">
        <!-- dynamic content here -->
    </div>
    
    </body>
    </html>

Given the following Dart code:

    import "dart:html";
    
    void main() {
    
        DivElement container = querySelector(".top-menu");
    
        container.appendHtml("Test");
    
    }

When compiling the Dart to JavaScript with the following command:

    dart2js --enable-checked-mode --analyze-all --enable-diagnostic-colors -o main.dart.js main.dart

When I visit the HTML page using regular Chrome I get the following error in the output:

    Uncaught type 'HtmlElement' is not a subtype of type 'DivElement' js_helper.dart:1058
    wrapException js_helper.dart:1058
    propertyTypeError js_helper.dart:2380
    interceptedTypeCheck js_helper.dart:2422
    main html_dart2js.dart:9665
    _IsolateContext.eval$1 isolate_helper.dart:247
    startRootIsolate isolate_helper.dart:67
    (anonymous function) main.dart.js:9515
    init.currentScript main.dart.js:9495
    (anonymous function) main.dart.js:9509
    (anonymous function) html_dart2js.dart:32250

Then I expect no errors and the div-element to contain the text "Test".

What version of the product are you using? On what operating system?

    Dart VM version: 1.1.3 (Thu Feb 06 00:06:36 2014) on "windows_ia32"
    Windows 8.1, 64bit

Live problem:

https://dl.dropboxusercontent.com/u/8183146/temp/dart/problem1/index.html
@floitschG
Copy link
Contributor

Added Area-HTML, Triaged labels.

@blois
Copy link
Contributor

blois commented Feb 25, 2014

I can repro this on 1.1, hitting another (apparently related) issue on 1.2.

On initial investigation it appears the tree-shaking by dart2js is too aggressive. This is falling through the cracks of our unit tests because package:unittest includes dart:mirrors which is essentially turning of tree shaking.

Need to eliminate the dart:mirrors import from package:unittest ASAP, in parallel.

Set owner to @rakudrama.
Removed Priority-Unassigned label.
Added Priority-Critical label.

@blois
Copy link
Contributor

blois commented Feb 25, 2014

Removed Area-HTML label.
Added Area-Dart2JS label.

@blois
Copy link
Contributor

blois commented Feb 25, 2014

Correction- tree shaking not caused by mock, but issue still appears related to tree shaking.

@DartBot
Copy link
Author

DartBot commented Feb 25, 2014

This comment was originally written by cor...@gmail.com

For the record, it seems like removing the "--analyze-all" flag seems to fix this problem as well (just as removing it fixes issue #9 @­17119).

@rakudrama
Copy link
Member

--analyze-all causes significant differences in the dart2js output.
This case is one where the difference breaks the program.
Most of the differences seem to cause lots of extra code to be generated.
For example, swarm grows by 112K.

It will take a concerted effort and extra testing to ensure --analyze-all does not affect the output.
We have never tested the executions of the output with --analyze-all.

It is probably wise, in the short term, to make --analyze-all generate only diagnostics and no JavaScript outputs.

Removed the owner.
Changed the title to: "dart2js: --analyze-all breaks code and generally generates different code".

@larsbak
Copy link

larsbak commented Feb 26, 2014

Set owner to @floitschG.

@johnniwinther
Copy link
Member

cc @floitschG.
Set owner to @johnniwinther.
Added Started label.

@johnniwinther
Copy link
Member

Fixed by https://codereview.chromium.org/185743002/ which makes --analyze-all imply --analyze-only.

Added Fixed label.

@peter-ahe-google
Copy link
Contributor

I'm concerned about the selected solution to address this bug:

* There is a bug in how type checks are implemented. Disabling tree-shaking during analysis shouldn't cause broken code. Making --analyze-all imply --analyze-only doesn't fix that problem.

* Using --analyze-all without --analyze-only is useful. It saves you time as compilation and analysis is done at the same time.

* I can construct a program that should behave equivalently by using mirrors (modulo a bug):

import 'dart:mirrors';

main() {
  var notTrue = false;
  if (notTrue) {
    print(reflect(1).invoke('toString', []).reflectee);
  }
}

I'm reopening this bug so my concerns don't drop of the radar, feel free to close it again. Also, I've removed the priority, it was critical which doesn't seem appropriate for my above concerns.

Removed Priority-Critical label.
Added Priority-Unassigned, Triaged labels.

@kevmoo kevmoo added type-enhancement A request for a change that isn't a bug type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) and removed priority-unassigned labels Feb 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@johnniwinther johnniwinther

Labels
type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) type-enhancement A request for a change that isn't a bug web-dart2js
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@kevmoo @sigmundch @rakudrama @blois @peter-ahe-google @larsbak @floitschG @DartBot @johnniwinther