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

add Date and other known globals to unsafe compress option #2302

Merged
merged 4 commits into from
Sep 6, 2017
Merged

add Date and other known globals to unsafe compress option #2302

merged 4 commits into from
Sep 6, 2017

Conversation

kzc
Copy link
Contributor

@kzc kzc commented Sep 5, 2017

We missed this one in #2236.

I think Date has been in javascript from the start.

@alexlamsl
Copy link
Collaborator

@kzc indeed - how did I miss that?!

Anyway, now that we are here, I wonder if things like parseInt() should be added as well:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects

@kzc
Copy link
Contributor Author

kzc commented Sep 6, 2017

Sure, we can add parseInt.

What about the following?

setTimeout
setInterval
clearTimeout
clearInterval
JSON

I don't think we should add the various typed Array types, nor should we add Map, Set, WeakMap and WeakSet - in uglify-js at least.

Which of the Error types should we add?

Error
EvalError
InternalError
RangeError
ReferenceError
SyntaxError
TypeError
URIError

@alexlamsl
Copy link
Collaborator

Hmm - global (side-effect-free) functions can't be optimised with the current code path, so I'll handle them in a separate PR.

As for the objects, I took the controversial step of testing if those mentioned on MDN exist under IE8:

EvalError
RangeError
ReferenceError
SyntaxError
TypeError
URIError
Date
JSON

@alexlamsl
Copy link
Collaborator

And since we have console, may be we should add self as well (window doesn't work within Web Worker)

@kzc
Copy link
Contributor Author

kzc commented Sep 6, 2017

Your list looks good - I'll update this PR. But I'll leave self out.

$ node0_10_41 -v
v0.10.41

$ node0_10_41 
> EvalError
[Function: EvalError]
> RangeError
[Function: RangeError]
> ReferenceError
[Function: ReferenceError]
> SyntaxError
[Function: SyntaxError]
> TypeError
[Function: TypeError]
> URIError
[Function: URIError]
> Date
[Function: Date]
> JSON
{}
> self
ReferenceError: self is not defined

@alexlamsl
Copy link
Collaborator

@kzc good catch & thanks! 😉

@kzc
Copy link
Contributor Author

kzc commented Sep 6, 2017

global (side-effect-free) functions can't be optimised with the current code path, so I'll handle them in a separate PR.

I thought global_names in lib/compress.js is just a list of known global symbols - unrelated to being side-effect-free:

    var global_names = makePredicate("Array Boolean console Error Function Math Number RegExp Object String");
    AST_SymbolRef.DEFMETHOD("is_declared", function(compressor) {
        return !this.definition().undeclared
            || compressor.option("unsafe") && global_names(this.name);
    });

Are you sure you don't want the following included in global_names as well?

setTimeout
setInterval
clearTimeout
clearInterval

@alexlamsl
Copy link
Collaborator

I'm referring to the fact that adding say parseInt to global_names alone won't make the common use case of parseInt(a) go away, as demonstrated by console:

$ echo 'console' | uglifyjs -c
console;

$ echo 'console' | uglifyjs -c unsafe


$ echo 'console()' | uglifyjs -c unsafe
console();

But I see your point about setTimeout without any function invocation being safe to be eliminated - same methodology as #2302 (comment) reveals:

eval
isFinite
isNaN
parseFloat
parseInt
decodeURI
decodeURIComponent
encodeURI
encodeURIComponent
escape
unescape

Further in https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope:

clearInterval
clearTimeout
setInterval
setTimeout

@kzc
Copy link
Contributor Author

kzc commented Sep 6, 2017

adding say parseInt to global_names alone won't make the common use case of parseInt(a) go away, as demonstrated by console

Indeed, I was aware of that. I see - you wanted an additional optimization for the pure call case.

I will incorporate your new list.

Any consequences to adding eval? It should be safe in the context of global_names I suppose.

@alexlamsl
Copy link
Collaborator

Any consequences to adding eval? It should be safe in the context of global_names I suppose.

Removing say var fn = eval; completely when fn is unused should be safe AFAICT.

@kzc
Copy link
Contributor Author

kzc commented Sep 6, 2017

There's a problem testing the following the test sandbox:

clearInterval;
clearTimeout;
setInterval;
setTimeout;

Those symbols do not exist.

Do we include them in global_names with no corresponding test? Or simply leave them out altogether from lib/compress.js?

@alexlamsl
Copy link
Collaborator

Can we make a separate test case without expect_stdout and include those there?

…l_timeout_and_interval_symbols as they do not exist in the test sandbox.
@kzc
Copy link
Contributor Author

kzc commented Sep 6, 2017

@alexlamsl Suggestions incorporated. PTAL.

@kzc
Copy link
Contributor Author

kzc commented Sep 6, 2017

Regarding #2302 (comment) - all these years I thought these functions would throw on bad input...

$ node
> parseInt(null)
NaN
> parseInt("QWERTY123")
NaN
> parseFloat("QWERTY123")
NaN

So they'd make fine pure functions in another PR.

@alexlamsl
Copy link
Collaborator

Your latest changes LGTM 👍

Most global functions in ES5 are very tolerant - so far I've only found these exceptions:

Function(1, 2);
RegExp(1, 2);
Array(NaN);

@kzc kzc changed the title add Date to known globals for unsafe compress option add Date and other known globals to unsafe compress option Sep 6, 2017
@alexlamsl alexlamsl merged commit 8b89072 into mishoo:master Sep 6, 2017
@kzc kzc deleted the Date-known-unsafe-global branch October 30, 2018 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants