NPE in RemoveUnusedCode pass

[robfig]

I'm getting a NPE in Closure Compiler when running it via the rules_closure Bazel rules. I have not succeeded in creating a small reproducer, so I was hoping for some direction to investigate further.

Imports

goog.declareModuleId('corp.searchanalytics.shared.tests.mediumpicker');

import {
  default as MediumPicker,
  getKey,
  AvailableItems,
  Buttons,
   OptionCategory,
  SelectedItems
} from '/src/com/corp/analytics/searchanalytics/searchanalyticsstorm/js/shared/mediumpicker';

The code resulting in the NPE:

describe('<MediumPicker />', () => {
  const onApplyMock = jest.fn();
  const onCancelMock = jest.fn();
  ... 
  it('renders without errors', () => {
    const container = React.createElement(MediumPicker, {  // <-- line 49, identified in the NPE
        ... 
      }
    );
  });

Exception:

java.lang.NullPointerException: NAME MediumPicker 49 [length: 12] [originalname: MediumPicker] [source_file: bazel-out/darwin-fastbuild/bin/src/com/.../js/shared/tests/mediumpicker_test.js] : ?
	at com.google.common.base.Preconditions.checkNotNull(Preconditions.java:895)
	at com.google.javascript.jscomp.RemoveUnusedCode.getVarForNameNode(RemoveUnusedCode.java:847)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseNameNode(RemoveUnusedCode.java:702)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseNode(RemoveUnusedCode.java:548)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseChildren(RemoveUnusedCode.java:1250)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseCall(RemoveUnusedCode.java:759)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseNode(RemoveUnusedCode.java:476)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseDeclarationStatement(RemoveUnusedCode.java:1032)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseNode(RemoveUnusedCode.java:528)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseChildren(RemoveUnusedCode.java:1250)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseFunction(RemoveUnusedCode.java:1386)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseNode(RemoveUnusedCode.java:446)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseChildren(RemoveUnusedCode.java:1250)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseCall(RemoveUnusedCode.java:759)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseNode(RemoveUnusedCode.java:476)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseChildren(RemoveUnusedCode.java:1250)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseNode(RemoveUnusedCode.java:557)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseChildren(RemoveUnusedCode.java:1250)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseFunction(RemoveUnusedCode.java:1386)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseNode(RemoveUnusedCode.java:446)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseChildren(RemoveUnusedCode.java:1250)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseCall(RemoveUnusedCode.java:759)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseNode(RemoveUnusedCode.java:476)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseChildren(RemoveUnusedCode.java:1250)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseNode(RemoveUnusedCode.java:557)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseChildren(RemoveUnusedCode.java:1250)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseNode(RemoveUnusedCode.java:557)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseChildren(RemoveUnusedCode.java:1250)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseNode(RemoveUnusedCode.java:557)
	at com.google.javascript.jscomp.RemoveUnusedCode.access$1300(RemoveUnusedCode.java:98)
	at com.google.javascript.jscomp.RemoveUnusedCode$Continuation.apply(RemoveUnusedCode.java:1728)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseAndRemoveUnusedReferences(RemoveUnusedCode.java:390)
	at com.google.javascript.jscomp.RemoveUnusedCode.process(RemoveUnusedCode.java:365)
	at com.google.javascript.jscomp.PhaseOptimizer$NamedPass.process(PhaseOptimizer.java:317)
	at com.google.javascript.jscomp.PhaseOptimizer.process(PhaseOptimizer.java:232)
	at com.google.javascript.jscomp.Compiler.performOptimizations(Compiler.java:2412)
	at com.google.javascript.jscomp.Compiler.lambda$stage2Passes$1(Compiler.java:797)
	at com.google.javascript.jscomp.CompilerExecutor$2.call(CompilerExecutor.java:102)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:834)

I thought that the import { default as X } syntax might not be supported, but a small reproducer seemed to show that working. I'm not sure what else could be the problem.

Thanks

[brad4d]

There's a check that runs early during compilation to ensure that all referenced variables are declared somewhere. However, when RemoveUnusedCode later runs and tries to find the declaration of MediumPicker it can't find one. This should never happen; thus the exception.

There's not enough context here for me to reproduce the problem or make a good guess at how this situation could have happened. Some questions I'd need answered include:

1. Is the import statement in the same file as the describe() call that comes later?
2. What does the imported file look like? Specifically the declaration of its default export.

[robfig]

1. Yes, the import and describe were from the same file (mediumpicker_test.js). In case it matters, it's an ES6 module, not goog.module.

2. Here's what the imported file looks like:

import array from 'goog:goog.array';
import json from 'goog:goog.json';
import string from 'goog:goog.string';
import object from 'goog:goog.object';

import Button from 'goog:corp.ui.components.Button';
import Input from 'goog:corp.ui.components.Input';

import { isAllTypeMediumType, getAllTypeByMediumType} from '/src/com/corp/analytics/searchanalytics/searchanalyticsstorm/js/data/mediumtype';
import MediumOption from '/src/com/corp/analytics/searchanalytics/searchanalyticsstorm/js/data/mediumoption';

export default function MediumPicker({
  onApply,
  onCancel,
  preselectedItems = [],
  items = [],
  isLoading,
}) {
    // ... lots of code ...
    return (
    React.createElement('div', { className: "medium-selector medium-picker-container" ,}
      , React.createElement(SelectedItems, {
         // etc
      }
   )

export function SelectedItems({ selectedItems, onReset, onRemoveItem }) { ... };
export function AvailableItems({ items, onSelect, onSelectAll, showLoadingSpinner }) { ... }
export function OptionCategory({ item, onSelect, onSelectAll }) { ... }
export function Buttons({ onCancel, onApply }) { ... }

Thank you for taking a look

[brad4d]

I don't see any smoking guns there. I think we'll need a minimal repro in order to diagnose.

I suggest you start by replacing the imported file with one that only has the export default with an empty function body. Does that still give the NPE?

[robfig]

OK, thanks for checking for smoking guns.

Replacing the imported file with

// mediumpicker.js
export default function MediumPicker() {}
export function getKey() {};
export function AvailableItems() {}
export function Buttons() {}
export function OptionCategory() {}
export function SelectedItems() {}

results in the same error, but commenting out any one of the imports makes the error go away:

// mediumpicker_test.js
import {
  default as MediumPicker,
  getKey,
  AvailableItems,
  Buttons,
  OptionCategory,
  // SelectedItems COMMENTED
} from '/src/com/corp/analytics/searchanalytics/searchanalyticsstorm/js/shared/mediumpicker';

Very very strange. I was not able to reproduce with a 2-file setup using a similar set of imports/exports.

I guess my next step would be building the compiler jar locally and adding a bunch of print statements.

[robfig]

I'm assuming that this is in the Compiler based on the trace, but it's possible this is an issue in the integration of the rules_closure ClosureWorker harness with a more recent Closure Compiler. Assuming nothing sticks out to you, I'll narrow it down further first. This is popping up as I'm trying to upgrade the Compiler from ~late 2019 to a more recent release. Thanks!

[brad4d]

@robfig you could try passing the --print_source_after_each_pass option to the compiler. It might then become apparent which pass is removing the definition of MediumPicker, or else that none is and something else must be the problem.

[robfig]

Thanks for the tip! Strangely, the MediumPicker source is not present even after first (parseInputs) pass. It got all the way to whatever pass is after earlyPeepholeOptimizations before aborting with the NPE, though.

I found another tweak that causes the code to work, which might be relevant. Previously, the code was doing this:

goog.declareModuleId('corp.searchanalytics.shared.tests.mediumpicker');

import ... 

and that module id was passed to the compiler as an entry point:

--entry_point=corp.searchanalytics.shared.tests.mediumpicker

I removed the declareModuleId and updated the entry point to

--entry_point=/src/com/corp/analytics/searchanalytics/searchanalyticsstorm/js/shared/tests/mediumpicker_test

that resulted in the MediumPicker definition being present and the tests passing.

[brad4d]

This does sound like the problem was a wrong entry point being provided to the compiler. I'm glad you figured it out.

[robfig]

Oh, I thought that the goog.declareModuleId was allowed to be used as an entry_point. This code currently works with an older Compiler, so the behavior must have changed. Are you saying it used to work as coincidence, but now any code relying on that needs to be updated?

[brad4d]

OK, honestly, I'm not sure. I'm flying blind here in several respects.

1. I don't know exactly what options you're passing to the compiler.
2. I don't know exactly what the source files look like.
3. I never use --entry_point because it's only used in open source projects.
4. I don't have the context of what setup you had that worked before and what you're changing that now breaks.

I read your last post as meaning "I accidentally treated mediumpicker.js as the entry point, instead of the test file which imports it." I guess that's not what you actually meant, so I'm not sure what's going on.

[robfig]

Yeah, I apologize for the vague bug report. I think I did get a simple reproducer that demonstrates the NPE as contrasted with an earlier closure compiler. Thanks for your help looking at this.

https://github.com/robfig/closure_compiler_repro

Clone that and run ./compile.sh

Out of curiosity, what do you use instead of --entry_point?

[brad4d]

Thanks a lot for the excellent repro. It took me less than 5 minutes to find the source of the problem with that in hand.

Change --entry_point corp.abc to --entry_point goog:corp.abc and it will build just fine.

If you want to use the closure namespace instead of a file path to specify the entry point, you have to use a goog: prefix.

See https://github.com/google/closure-compiler/wiki/Flags-and-Options#dependency-management

It's also true that this isn't a good failure mode, so I won't close this issue.

Seems like it ought to complain that it couldn't find a file named corp.abc.

Out of curiosity, what do you use instead of --entry_point?

Within Google the command line interface is different and also mostly hidden behind build rules that know the right options to generate to specify input files.

[robfig]

Ah, thank you for the explanation, and sorry for wasting your time prior to finding a good reproducer!