review: fix: do not remove constructor parameters from inner classes

[SirYwell]

Previously, one parameter was deleted for each nesting level. However, only one parameter exists for the direct enclosing class.

[SirYwell]

@monperrus @slarse could one of you review this? @MartinWitt co-authored the fix, therefore someone else should take a second look at it. Thanks!

[slarse]

@monperrus @slarse could one of you review this? @MartinWitt co-authored the fix, therefore someone else should take a second look at it. Thanks!

I can have a look tonight.

[slarse]

@SirYwell I'm a little bit confused about Parameter.isImplicit(), see question in code.

[slarse]

Looking at the coverage, this branch is never taken by the tests. Yet I would expect the test added to execute this branch as the code is compiled with Java >8. Trying this locally with JDK 11 (both OpenJDK and Amazon Coretto), isImplicit() returns false for clearly implicit parameters.

@SirYwell can you explain this to me?

[SirYwell]

It's complicated. Since Java 8, the JLS says that

12. A construct emitted by a Java compiler must be marked as mandated if it corresponds to a formal parameter declared implicitly in source code

(JLS 13.1)

However, javac only does this when compiling with the -parameters flag. See https://mail.openjdk.java.net/pipermail/compiler-dev/2022-May/019783.html for more context.

So ih theory, it's the best way to figure out if a parameter is implicit, but in practice, it would require the compiler to keep to the spec.

[slarse]

I see. Then I suggest extracting the implicitness check into a separate method and put all of the relevant information required to understand why it's so funky in the Javadoc (for example the stuff you wrote above). Something like this:

/**
 * Determine if the constructor parameter is is implicit.
 *
 * This is a great description of why this is so weird.
 */
private boolean isImplicitParameter(Parameter parameter, Constructor constructor, boolean isFirstParameter) {
    // do isImplicit() and "best guess" checks
}

It doesn't have to be exactly like that, but just provide any information required into the method such that it can perform the checks. The implementation of Constructor.getParameters() looks rather heavy so we probably don't want to call it multiple times.

There are a lot of places in Spoon where weird corner cases aren't adequately explained, so I'm trying to make a push for... explaining them.

[SirYwell]

I extracted the relevant code into its own method now.

[slarse]

I see. Then I suggest extracting the implicitness check into a separate method and put all of the relevant information required to understand why it's so funky in the Javadoc (for example the stuff you wrote above). Something like this:

/**
 * Determine if the constructor parameter is is implicit.
 *
 * This is a great description of why this is so weird.
 */
private boolean isImplicitParameter(Parameter parameter, Constructor constructor, boolean isFirstParameter) {
    // do isImplicit() and "best guess" checks
}

It doesn't have to be exactly like that, but just provide any information required into the method such that it can perform the checks. The implementation of Constructor.getParameters() looks rather heavy so we probably don't want to call it multiple times.

There are a lot of places in Spoon where weird corner cases aren't adequately explained, so I'm trying to make a push for... explaining them.

[slarse]

Thanks @SirYwell, looks good to me! I've left an optional comment that you can do something about if you'd like, but it's a tiny comment and I'm perfectly fine with merging this as-is if you disagree (but let me know which one you decide on).

[slarse]

Optional comment: You could contract this into a single return statement.

Suggested change

	if (isFirstParameter && parameter.getType() == constructor.getDeclaringClass().getEnclosingClass()) {
	return true;
	}
	return false;
	return isFirstParameter && parameter.getType() == constructor.getDeclaringClass().getEnclosingClass()

IMO it's better to just return like this, but I've met people who disagree so I'll leave it up to you if you want to change it or not.

[SirYwell]

I applied the suggested change.

[slarse]

Thanks @SirYwell. This was deceptively complicated.

[monperrus]

Thanks!