Review: feat: add JLSViolation exception

[MartinWitt]

As discussed in #4134 we need a more specific exception for JLS correctness. See the record pr for the reason behind this.

[monperrus]

Interesting and well-scoped baby PR.

• see comment
• we need a test case

Thanks!

[monperrus]

This method is not related to the element's state. I feel we should have it elsewhere.

Proposal: as static method in SpoonException?

[MartinWitt]

The problem with moving it away form CtElement is that we need the environment or the factory for the check if we need(the user wants) to throw the exception. I'm unsure if passing a factory or environment to the exception creation produces good code.

public void throwJLSViolation(Factory fac, String reason)

looks a bit strange to me with the knowledge, the factory is not used for the exception message.

[slarse]

I agree with @monperrus, this method should not be on CtElement. It's a helper method for Spoon internals, exposing it in the public API is not desirable.

I also don't really like the idea of having a method named throwX, that may not actually throw X. A better name would probably be something like handleJLSViolation, but I'm not entirely sold on the necessity for the method to begin with.

[MartinWitt]

I moved the Exception to SpoonException in a static until method.

but I'm not entirely sold on the necessity for the method to begin with.

The goal of this method is to not repeat the check if the exception should be logged or thrown at every place we have JLSViolations.

[slarse]

This concept is extremely close to setIgnoreSyntaxErrors. A JLS violation is in most situations a syntax error. If this is to be added as a separate flag, it needs to be clearly distinguished from setIgnoreSyntaxErrors.

The key thing about setIgnoreSyntaxErrors is that it's a filter to ignore files with syntax errors in the input. This new proposed flag could perhaps distinguish itself by being explicitly about maintaining JLS compliance in the Spoon model.

What I'm trying to get across is that if I were to read these docs, the distinction between this flag and setIgnoreSyntaxErrors would not be clear to me.

Finally, I'd argue this should be called "ignore JLS violations" or "skip JLS correctness checks". I understand what "ignore correctness" is meant to imply, but it's a bit of a brain twister for me.

[MartinWitt]

	/**
	 * Sets the option ignore-syntax-errors to remove files with any syntax errors from the compilation batch.
	 *
	 * @param ignoreSyntaxErrors whether Spoon should ignore files with any syntax errors or JLS violations
	 */
	void setIgnoreSyntaxErrors(boolean ignoreSyntaxErrors);

After reading these docs, I think there is a problem with merging these 2 flags, because building a model with a wrong identifier is possible, but the flag removes the file completely. I'm unsure if ignoring = removing is expected behavior.

[monperrus]

About the concept here there are two things.

• introducing a new exception
• having this as configurable behavior

The former is OK to me. We can already put in this PR some usages.

As @slarse I'm not enthusiastic about the latter because 1) configurable behavior is hard to understand for clients 2) configurable behavior is harder to test and maintain 2) configurable behavior for exceptions is especially caveat.

WDYT?

[MartinWitt]

My intention with allowing non JLS compliant code was that some (maybe existing) transformations have steps which produce non JLS code. I think we shouldn't remove the ability to write transformations freely. But as @slarse pointed out correctly, the JLS correctness is pretty close to tIgnoreSyntaxError and the existing flag should be used.

We can already put in this PR some usages.

This could be hard as Spoon ignores a lot of JLS restrictions and is more focused on building any model.
Edit: We could change the identifier checks to throw JLSViolation

[MartinWitt]

Here we could say ignoring means no logging at all. Any opinions on just log it, even if the user said ignoreSyntaxErrors?

[slarse]

It's fine to log IMO.

[monperrus]

Thanks for the update. LGTM.

[slarse]

See my comment on the code.

In addition, this changes the semantics of setIgnoreSyntaxErrors, and its docs must be updated. It's no longer only about ignoring syntax errors in files, but also in transformations, which must be expressed in the docs.

[slarse]

It's fine to log IMO.

[slarse]

I don't think this method belongs here either. This makes SpoonException dependent on its subtype, which is undesirable and IMO violates SRO (SpoonException might have to change if JLSViolation changes).

If this method should exist, and should be the prime way of throwing a JLSViolation, I would put it as a static method on JLSViolation and make its constructor private. I.e. like so:

public class JLSViolation extends SpoonException {

	private JLSViolation(String msg) {
		super(msg);
	}

	public static void throwIfSyntaxErrorsAreNotIgnored(FactoryAccessor holder, String reason) {
		// logic    
	}

}

Then usage becomes a bit easier to read, I think:

// current
SpoonException.handleJLSViolation(...);

// my proposition
JLSViolation.throwIfSyntaxErrorsAreNotIgnored(...);

What do you think? Agree, disagree?

[MartinWitt]

100% correct, I moved the method and changed the name to your better name.

[slarse]

LGTM

I think using more specific exceptions is a step in the right direction for Spoon overall.

[slarse]

Thanks @MartinWitt