closure-compiler icon indicating copy to clipboard operation
closure-compiler copied to clipboard

ADVANCED_OPTIMIZATION - inlining functions that are declared as @externs - and so breaks the externs.

Open KnickdaKnife opened this issue 1 year ago • 3 comments

When compiling a Function to be called from External code, and declaring the Function in the @externs file so it will be retained without being renamed, this works fine for sufficiently complicated functions. However when the function is very simple, the compiler decides it can be inlined - after which it is no longer present as a function in the compiled code, and so it is no longer available as a @extern.

Tests carried out using the REST API https://closure-compiler.appspot.com/compile with output_format=json& output_info=errors&output_info=warnings&output_info=statistics&output_info=compiled_code& warning_level=default& compilation_level=ADVANCED_OPTIMIZATIONS& language=ECMASCRIPT5_STRICT& js_code= <Combined-v1_0.pac attached below>& js_externs= <Combined_JSExterns.js attached below>

The output was Combined.pac - also attached, along with a manually expanded version of that Combined-compiled.pac where we can see that the missing function was inlined by the Compiler. Combined-evidence.zip

KnickdaKnife avatar Sep 27 '22 14:09 KnickdaKnife

I think that I have managed to reproduce the issue in the Debugger. I hope this helps provide evidence of the faulty behaviour - which I think is a BUG.

https://closure-compiler-debugger.appspot.com/#input0%3D%252F**%2520%250A*%2520%2540suppress%2520%257Bduplicate%257D%2520*%252F%250Avar%2520PACfilename%253D%2522Combined-v1_0.pac%2522%253B%250A%250A%252F%252F%2520Setup%2520persistent%2520variables%2520that%2520are%2520initialised%2520on%2520first%2520use%2520of%2520PAC%2520file.%250Avar%2520sStandardProxy%252C%2520sWork%253B%250A%250Aif(typeof%2520alert%253D%253D%253D%2522function%2522)%2520%257BsWork%253DFindProxyForURL(%2522%2522%252C%25221.2.3.4%2522)%253Balert(PACfilename%252B%2522%2520Function1%253D%2522%252BsWork%252B%2522%2520%2526%2520%2522%252BsStandardProxy)%253B%2520%257D%250Aif(typeof%2520alert%253D%253D%253D%2522function%2522)%2520%257BsWork%253DFindProxyForURL_noinline(%2522%2522%252C%25221.2.3.4%2522)%253Balert(PACfilename%252B%2522%2520Function2%253D%2522%252BsWork%252B%2522%2520%2526%2520%2522%252BsStandardProxy)%253B%2520%257D%250Aif(typeof%2520alert%253D%253D%253D%2522function%2522)%2520%257BsWork%253DFindProxyForURL_complex(%2522%2522%252C%25221.2.3.4%2522)%253Balert(PACfilename%252B%2522%2520Function3%253D%2522%252BsWork%252B%2522%2520initialised%2520%2522%252BsStandardProxy)%253B%2520%257D%250A%250A%252F**%2520%250A*%2520%2540param%2520%257Bstring%257D%2520Url%2520%2540param%2520%257Bstring%257D%2520Host%2520%2540return%2520%257Bstring%257D%2520%2540suppress%2520%257Bduplicate%257D*%252F%250Afunction%2520FindProxyForURL(Url%252C%2520Host)%2520%257B%250Areturn%2520%2522PROXY%2520165.225.196.13%253A8800%2522%253B%2509%250A%257D%250A%250A%252F**%2520Adding%2520the%2520(at)%2520noinline%2520to%2520keep%2520it%250A*%2520%2540param%2520%257Bstring%257D%2520Url%2520%2540param%2520%257Bstring%257D%2520Host%2520%2540return%2520%257Bstring%257D%2520%2540suppress%2520%257Bduplicate%257D%2520%2540noinline%2520*%252F%250Afunction%2520FindProxyForURL_noinline(Url%252C%2520Host)%2520%257B%250Areturn%2520%2522PROXY%2520165.225.196.13%253A8800%2522%253B%2509%250A%257D%250A%250A%252F**%2520More%2520complex%2520function%2520is%2520kept%250A*%2520%2540param%2520%257Bstring%257D%2520Url%2520%2540param%2520%257Bstring%257D%2520Host%2520%2540return%2520%257Bstring%257D%2520%2540suppress%2520%257Bduplicate%257D*%252F%250Afunction%2520FindProxyForURL_complex(Url%252C%2520Host)%2520%257B%250A%252F%252F%2520Define%2520constants%2520for%2520Captive%2520Portal%2520Check%250Avar%2520PORTAL%253D0%252C%2520NORMAL%253D1%252C%2520RESILIENCE%253D2%2520%253B%250A%250A%252F%252F%2520Define%2520constants%250Avar%2520sFALLBACK%2520%253D%2520%2522PROXY%2520165.225.196.13%253A8800%2522%253B%2509%252F%252F%2520An%2520accessible%2520Proxy%2520in%2520another%2520zScaler%2520Cloud.%250A%250A%252F%252F%2520Working%2520variables%250Avar%2520iCase%252C%2520sReturn%253B%250A%250A%252F%252F%2520Choose%2520London-Manchester%2520pairs%252C%2520alternating%2520(sStandardProxy)%250Aif(!sStandardProxy)%2520%257BsplitProxy(%255B0%252C1%252C2%252C4%255D)%253B%2520%257D%250A%250A%252F%252F%2520Check%2520for%2520Captive%2520Portal%2520%252F%2520Resilience%2520using%2520DNS%2520resolution%2520of%2520Internal%2520Name%2520%250A%250AiCase%253DcheckCaptivePortal()%253B%250Aif(iCase%253D%253D%253DPORTAL)%2520%257BsReturn%253D%2522DIRECT%253B%2522%253B%2520%257D%2520%252F%252F%2520Captive%2520Portal%2520detected%250Aelse%2520if(iCase%253D%253D%253DRESILIENCE)%2520%257BsReturn%253DsFALLBACK%253B%2520%257D%2520%252F%252F%2520Use%2520Fallback%2520for%2520Resilience%250Aelse%2520%257BsReturn%253DsStandardProxy.replace(%252F%253A%252Fg%252C%2522%253A10558%2522)%253B%2520%257D%250A%250Areturn%2520sReturn%253B%250A%250Afunction%2520dns2IP(name)%2520%257B%2520return%2520dnsResolve(name)%253B%2520%257D%250Afunction%2520myIP()%2520%257B%2520return%2520myIpAddress()%253B%2520%257D%250A%250Afunction%2520checkCaptivePortal()%2520%257Bvar%2520sIP%253Ddns2IP(%2522zscaler-proxy-check.customer.org.uk%2522)%253B%250A%2509return(sIP%253D%253D%253D%252210.144.3.13%2522%253FNORMAL%253AsIP%253D%253D%253D%252210.144.3.15%2522%253FRESILIENCE%253APORTAL)%253B%2520%257D%250A%250Afunction%2520splitProxy(iUse)%2520%250A%257Bvar%2520GW%253D%255B%2522PROXY%2520zs-165-225-%2522%252C%2520%2522.gateway.zscloud.net%253A%2522%255D%252C%2520iVIP%253DiUse.length%252C%2520iRand%253DMath.floor(Math.random()iVIP2)%253B%250A%2509var%2520London%253DGW%255B0%255D%252B%252216-%2522%252B(237%252BiUse%255BiRand%2525iVIP%255D)%252BGW%255B1%255D%252C%2520Manchester%253DGW%255B0%255D%252B%2522196-%2522%252B(39%252BiUse%255BiRand%2525iVIP%255D)%252BGW%255B1%255D%253B%250A%2509sStandardProxy%253D(iRand%253CiVIP%253FLondon%252B%2522%253B%2522%252BManchester%253AManchester%252B%2522%253B%2522%252BLondon)%253B%250A%257D%250A%250A%257D%26input1%26conformanceConfig%26externs%3D%252F**%2520%2540fileoverview%2520Externs%2520definitions%2520for%2520PAC%2520file%250A%2520*%2520%2540externs%2520*%252F%250A%252F*%250A%2520*%2520%2540param%2520%257Bstring%257D%2520Url%2520%2540param%2520%257Bstring%257D%2520Host%2520%2540return%2520%257Bstring%257D%2520*%252F%250Afunction%2520FindProxyForURL(Url%252C%2520Host)%2520%257B%257D%253B%250A%250A%252F**%2520%2520The%2520test%2520file%2520has%2520two%2520more%2520example%2520%250A%2520*%2520%2540param%2520%257Bstring%257D%2520Url%2520%2540param%2520%257Bstring%257D%2520Host%2520%2540return%2520%257Bstring%257D%2520*%252F%250Afunction%2520FindProxyForURL_noinline(Url%252C%2520Host)%2520%257B%257D%253B%250A%250A%252F**%2520%2520The%2520final%2520example%2520is%2520too%2520complex%2520for%2520inlining.%250A%2520*%2520%2540param%2520%257Bstring%257D%2520Url%2520%2540param%2520%257Bstring%257D%2520Host%2520%2540return%2520%257Bstring%257D%2520*%252F%250Afunction%2520FindProxyForURL_complex(Url%252C%2520Host)%2520%257B%257D%253B%250A%250A%252F**%2520Retain%2520the%2520name%2520of%2520the%2520source%2520file%250A%2520*%2520%2540type%2520%257Bstring%257D%2520%2520*%252F%250Avar%2520PACfilename%253B%250A%26refasterjs-template%26includeDefaultExterns%3Dtrue%26AMBIGUATE_PROPERTIES%3Dtrue%26COALESCE_VARIABLE_NAMES%3Dtrue%26COLLAPSE_VARIABLE_DECLARATIONS%3Dtrue%26COLLAPSE_ANONYMOUS_FUNCTIONS%3Dtrue%26COLLAPSE_PROPERTIES%3Dtrue%26COLLAPSE_OBJECT_LITERALS%3Dtrue%26COMPUTE_FUNCTION_SIDE_EFFECTS%3Dtrue%26CONVERT_TO_DOTTED_PROPERTIES%3Dtrue%26CROSS_CHUNK_CODE_MOTION%3Dtrue%26CROSS_CHUNK_METHOD_MOTION%3Dtrue%26DEAD_ASSIGNMENT_ELIMINATION%3Dtrue%26DEVIRTUALIZE_METHODS%3Dtrue%26DISAMBIGUATE_PROPERTIES%3Dtrue%26EXTRACT_PROTOTYPE_MEMBER_DECLARATIONS%3Dtrue%26FOLD_CONSTANTS%3Dtrue%26INLINE_CONSTANTS%3Dtrue%26INLINE_FUNCTIONS%3Dtrue%26INLINE_PROPERTIES%3Dtrue%26INLINE_VARIABLES%3Dtrue%26LABEL_RENAMING%3Dtrue%26OPTIMIZE_CALLS%3Dtrue%26OPTIMIZE_CONSTRUCTORS%3Dtrue%26OPTIMIZE_ARGUMENTS_ARRAY%3Dtrue%26REMOVE_ABSTRACT_METHODS%3Dtrue%26REMOVE_DEAD_CODE%3Dtrue%26REMOVE_UNUSED_CLASS_PROPERTIES%3Dtrue%26REMOVE_UNUSED_PROTOTYPE_PROPERTIES%3Dtrue%26REMOVE_UNUSED_VARIABLES%3Dtrue%26REWRITE_FUNCTION_EXPRESSIONS%3Dtrue%26SMART_NAME_REMOVAL%3Dtrue%26USE_TYPES_FOR_LOCAL_OPTIMIZATION%3Dtrue%26VARIABLE_RENAMING%3Dtrue%26PROPERTY_RENAMING%3Dtrue%26SYNTHETIC_BLOCK_MARKER%3Dtrue%26CLOSURE_PASS%3Dtrue%26CONTINUE_AFTER_ERRORS%3Dtrue%26PRETTY_PRINT%3Dtrue

KnickdaKnife avatar Oct 05 '22 13:10 KnickdaKnife

Using the Debugger again, I found that by disabling the INLINE_FUNCTIONS Optimization, I also blocked the unwanted behaviour (though of course then NO functions were inlined and my code is not so well optimized).

KnickdaKnife avatar Oct 05 '22 13:10 KnickdaKnife

I agree this is a bug, but you might consider using a different strategy for making your functions available to external code by explicitly declaring them on the global object. We don't really recommend declaring the same variable in both code and externs as seen by the @suppress {duplicate} you had to add.

For example:

/** @param {string} str */
function foo(str) {
  return str;
}

alert(foo('test'));

window['fooExternalName'] = foo;

This optimizes nicely to:

alert("test");
window.fooExternalName = function(a) {
  return a;
};

debugger link

lauraharker avatar Oct 05 '22 23:10 lauraharker