build-optimizer: "logical OR" property assignment is removed

@filipesilva

Bug Report or Feature Request (mark with an `x`)

- [X] bug report -> please search issues before submitting
- [ ] feature request

Area

- [X] devkit
- [ ] schematics

Versions

`-- @angular/cli@1.6.4
  `-- @angular-devkit/build-optimizer@0.0.38

node -v // 8.5.0
npm -v // 5.5.1

Repro steps

Clone this reproduction repository and run the app in two different ways:

With the build optimizer (using -prod --aot)
Without the build optimizer (using -prod --aot --build-optimizer=false)

The highlighted code snippet here (picture from the ESM 5 file button.js file) is being removed when running with the build optimizer. If you add a console.log before, the console.log stays, just the specific code snippet is being removed.

Switching to a ternary, or simple if-else will fix the issue.

with build-optimizer disabled

_with build-optimizer enabled

Mention any other details that might be useful

Related to Angular Material. angular/components#9360

cc. @filipesilva

fix(button): accent color not set on fab buttons with build optimizer

@IgorMinar

I'm looking into this problem and I think the current behaviour, although not very intuitive, is correct.

This is a situation similar to #364, of which the most important bit is:

This is expected behaviour since Build Optimizer consider all code in @angular/* packages to be side-effect free. This causes all top level function calls and constructor to get the /*@__PURE__*/ comment, which will later cause those statements to be dropped if their values are not used.

In this case, the original code

var MatFab = /** @class */ (function () {
    function MatFab(button, anchor) {
        // Set the default color palette for the mat-fab components.
        (button || anchor).color = DEFAULT_ROUND_BUTTON_COLOR;
    }
    MatFab.decorators = [
        { type: Directive, args: [{
                    selector: 'button[mat-fab], a[mat-fab]',
                    host: { 'class': 'mat-fab' }
                },] },
    ];
    /** @nocollapse */
    MatFab.ctorParameters = function () { return [
        { type: MatButton, decorators: [{ type: Self }, { type: Optional }, { type: Inject, args: [forwardRef(function () { return MatButton; }),] },] },
        { type: MatAnchor, decorators: [{ type: Self }, { type: Optional }, { type: Inject, args: [forwardRef(function () { return MatAnchor; }),] },] },
    ]; };
    return MatFab;
}());

will be modified to

var MatFab = /*@__PURE__*/ (function () {
    function MatFab(button, anchor) {
        (button || anchor).color = DEFAULT_ROUND_BUTTON_COLOR;
    }
    return MatFab;
}());

A couple of things are happening here:

decorators and ctorParameters are removed as per the scrub-file transform
the class declaration is marked as side effect free as per the prefix-functions transform

The less obvious effect of this is that UglifyJS (with pure_getters=true, which we use) will later completely remove (button || anchor).color = DEFAULT_ROUND_BUTTON_COLOR;.

However, looking a bit more into what being side effect free means, this makes sense.

Consider this snippet:

// test.js
var MatFab = /*@__PURE__*/ (function () {
  function MatFab(button, anchor) {
    (button || anchor).color = 'accent';
  }
  return MatFab;
}());
var button = {};
MatFab(button);
console.log(button);

One would expect it to print {color: "accent"}.

But if you run it through uglify (uglifyjs -c toplevel,pure_getters=true -b -- test.js) you will get:

var button = {};

(function() {
    return function(button, anchor) {};
})()(button), console.log(button);

And here you see it does not set the property anymore.

We say MatFab is side effect free through the /*@__PURE__*/ comment. This means that, when its return value is not used, the statement can safely be dropped because it did not have any side effects. But modifying a property of an argument is a side effect, so MatFab is not actually side effect free.

This is the second problema related to side effects we've had in the last week and I think there will only be more in the future unless an automated way to detect these is implemented. I'm not sure that's even possible, but maybe some linting rules (e.g. don't mutate arguments or call void functions) will go a long way.

/cc @IgorMinar @petebacondarwin

Really this should be catchable by running the unit tests for the library code on the "purified" code, right?

For the future (in this case), would it be possible to parse the code and look for mutations to passed in parameters; and so not mark as PURE?

I think running unit tests on purified code is really smart and should work. We can setup our own pipeline for that, but it makes me think tooling should be able to do this sort of thing in the future.

I think it's possible to detect these things, and UglifyJS does it, but in this case we specifically set the pure_getters=true option. It means that property access is always side effect free. If you take it out then the statement is not removed anymore. But this property is responsible for a big part of our size savings.

I don't understand several things. Sorry for stupid questions:

(button || anchor).color = DEFAULT_ROUND_BUTTON_COLOR; is a setter so why does pure_getters=true matter? are we sure that the uglify option is responsible for the assignment getting DCE-ed?
what does this code look like in it's original form? can you link to a snippet from the material repo?

fix(button): accent color not set on fab buttons with build optimizer

@IgorMinar

@IgorMinar this is how the original code looks like: https://github.com/angular/material2/blob/45235566588805ebb06f59f9b7826b865429ec3e/src/lib/button/button.ts#L77-L83

@devversion has a PR to change this logic (angular/components#9376).

Regarding pure_getters being responsible, I tested using the same test input:

// test.js
var MatFab = /*@__PURE__*/ (function () {
  function MatFab(button, anchor) {
    (button || anchor).color = 'accent';
  }
  return MatFab;
}());
var button = {};
MatFab(button);
console.log(button);

And varying the pure_getters=true option:

uglifyjs -c toplevel,pure_getters=true -b -- test.js:

var button = {};

(function() {
    return function(button, anchor) {};
})()(button), console.log(button);

uglifyjs -c toplevel -b -- test.js:

var MatFab = function() {
    return function(button, anchor) {
        (button || anchor).color = "accent";
    };
}(), button = {};

MatFab(button), console.log(button);

It might also be the interaction of pure_getters with a default option though.

It's also relevant to notice that changing (button || anchor).color = 'accent'; to just button.color = 'accent'; will not cause the statement to be dropped even with pure_getters=true. I don't really understand why this last bit changes behaviour.

If I had to bet, the expression inside parentheses causes UglifyJS to consider the left hand expression as a getter instead of a setter. This might be a UglifyJS bug/limitation (/cc @kzc).

But even if that's the case, having a function that alters one of the arguments and returns nothing seems like a side effect to me. Retaining the function call would be a limitation of side-effect detection in UglifyJS.

The bug should still be fixed because it's unintended behaviour for the option though.

@filipesilva

@filipesilva Please report this to the uglify project as an ES5 bug.

Keep in mind that pure_getters is a non-default option.

It appears to be a regression from uglify-js@3.2.2. The commit that introduced the issue is mishoo/UglifyJS@0b0eac1

@kzc

@kzc opened as mishoo/UglifyJS#2838 👍

The UglifyJS bug bit of this issue was fixed in mishoo/UglifyJS#2839 and should be out on the next release.

fix(button): accent color not set on fab buttons with build optimizer (…

Awesome! That makes so much more sense because I kept on looking at the code and wondering why the assignment was getting stripped. Sanity restored.

I do have to say that this code, particularly the constructor with non-local side-effect, is very odd looking and while it doesn't break our optimizations it should raise some eyebrows.

fix(button): accent color not set on fab buttons with build optimizer (…

Closing as the devkit issue is sorted, as far as I can tell.

Navigation Menu

Repository navigation

build-optimizer: "logical OR" property assignment is removed #388

Bug Report or Feature Request (mark with an `x`)

Area

Versions

Repro steps

Mention any other details that might be useful

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

build-optimizer: "logical OR" property assignment is removed #388

Description

Bug Report or Feature Request (mark with an x)

Area

Versions

Repro steps

Mention any other details that might be useful

Activity

filipesilva commented on Jan 15, 2018

petebacondarwin commented on Jan 15, 2018

filipesilva commented on Jan 15, 2018

IgorMinar commented on Jan 19, 2018

filipesilva commented on Jan 22, 2018

kzc commented on Jan 22, 2018

kzc commented on Jan 22, 2018

filipesilva commented on Jan 22, 2018

filipesilva commented on Jan 23, 2018

IgorMinar commented on Jan 24, 2018

filipesilva commented on Jan 30, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Bug Report or Feature Request (mark with an `x`)