Refactor EclipseSourceFileModule to make it more extension-friendly

[khatchad]

The removed null checks seem to happen further up the class hierarchy. Replace the static construction method with a public constructor. A lone private constructor basically makes it basically impossible to subclass.

[github-actions]

Test Results

  572 files  ±0    572 suites  ±0   3h 41m 12s ⏱️ +12s
  734 tests ±0    717 ✅ ±0  17 💤 ±0  0 ❌ ±0 
3 554 runs  ±0  3 467 ✅ ±0  87 💤 ±0  0 ❌ ±0 

Results for commit 1f791e3. ± Comparison against base commit 3a2ca2e.

♻️ This comment has been updated with latest results.

[msridhar]

I understand making the constructor public, but not the removal of the static method and the null checks. Why do we need the latter?

[khatchad]

I understand making the constructor public, but not the removal of the static method and the null checks. Why do we need the latter?

I believe that the reason the ctor was originally made private was so that the null check can be enforced. The exception throw couldn't be done in the ctor because super() needs to be the first statement. So, we have the static method invoking a private constructor.

If we make the ctor public, then you don't need the static method because you can simply just call the public ctor. So, I attempted to move the null checks to the ctor. I think this can be done as the null checking actually happens already further up the class hierarchy.

We could keep both the (now public) ctor and the static method, but I don't think anyone would use the latter if the former is available.

[msridhar]

Can you add a comment here indicating that passing null to super will cause a failure in the FileModule constructor? We should really get NullAway running on all this code to get rid of this kind of stuff :-)

[khatchad]

Does it cause a failure? We would hit this code:

WALA/core/src/main/java/com/ibm/wala/classLoader/FileModule.java

Lines 30 to 32 in 3a2ca2e

	if (f == null) {
	throw new IllegalArgumentException("f is null");
	}

Which is what we had previously. The only thing that would really be different is that the filename string would be null.

[khatchad]

Or, is the thought that it was clear in this file that an exception would be thrown in this case, and now with the new code, we would need to go two levels up to see the same thing?

[msridhar]

My point was that it'd be good to write a comment indicating that we deliberately pass null knowing that there is logic to fail elsewhere, so a reader does not get confused and think it is valid to pass null to the super constructor

[khatchad]

I understand making the constructor public, but not the removal of the static method and the null checks. Why do we need the latter?

I believe that the reason the ctor was originally made private was so that the null check can be enforced. The exception throw couldn't be done in the ctor because super() needs to be the first statement. So, we have the static method invoking a private constructor.

If we make the ctor public, then you don't need the static method because you can simply just call the public ctor. So, I attempted to move the null checks to the ctor. I think this can be done as the null checking actually happens already further up the class hierarchy.

We could keep both the (now public) ctor and the static method, but I don't think anyone would use the latter if the former is available.

Perhaps a different way to put it is that it looked like the static method is encapsulating the object construction. But that encapsulation is too strong if we want to subclass.