Add Check Support for Java 17 Sealed Classes: IllegalType

[mahfouz72]

child of #14969
I have read check documentation: https://checkstyle.org/checks/coding/illegaltype.html#IllegalType
I have downloaded the latest checkstyle from: https://checkstyle.org/cmdline.html#Download_and_Run
I have executed the cli and showed it below, as cli describes the problem better than 1,000 words

PS D:\CS\test> cat src/Test.java                                                 

class IllegalType { }
public class Test extends IllegalType {            // violation
    public sealed class A permits IllegalType2 { }  // expected violation
    final class IllegalType2 extends A { }
}
PS D:\CS\test> cat config.xml                                                    
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
        "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
        "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>
    <module name="TreeWalker">
         <module name="IllegalType">
            <property name="illegalClassNames" value="IllegalType, IllegalType2"/>
        </module>

    </module>
</module>
PS D:\CS\test> java  -jar checkstyle-10.17.0-all.jar -c config.xml src/Test.java 
Starting audit...
[ERROR] D:\CS\test\src\Test.java:3:27: Usage of type 'IllegalType' is not allowed. [IllegalType]
Audit done.
Checkstyle ends with 1 errors.
PS D:\CS\test> 

---

Describe what you expect in detail.

violation on the Illegal classes in the permitted list

---

[nrmancuso]

I keep coming back to this one, and feel like it is kind of weird, so I am having a hard time decided what to do here. In practice, I would use IllegalType for the following:

1. To ban usage of some deprecated class in a codebase
2. To ban usage of some library class in a codebase

However, with permits, we would not have to worry about case (2) above, since we can safely assume that we have control over any class that we permit. For (1), I suppose this is possible. So - I am leaning towards approving this issue, but would like a second opinion from @rnveach

[rnveach]

To help clarify things, this only seems to violate declared types, it won't violate instance usages of the type.

Example from https://checkstyle.org/checks/coding/illegaltype.html#Example1-config :

    LinkedHashMap<Integer, String> m = // violation
        new LinkedHashMap<Integer, String>(); // no violation

    Map<Integer, String> m =
        new LinkedHashMap<Integer, String>(); // no violation

It will ban the class from being extended, as that is being declared as a sub-type.

I think this may need to be added to the documentation to help make this check clearer.

In practice, I would use IllegalType for the following:

I mainly use ImportControl for as a way to ban things and my reasoning is sort of different from your 2.

I had a project with sub-projects which acted as tiers. New developers would try to import classes from other tiers and then not understand why their code wasn't working. They did not realize these tiers were more physically separated and couldn't be cross called at all times. There were certain reasons that the tiers could even be imported like this and they weren't fully disabled. So I used a banned list to help prevent crossing tiers where it wasn't appropriate.

I actually think there is a 3rd reason, which is the more main reason, for this type of check.

This check seems to be more centered around banning concrete classes and force the user to use the interfaces instead. The types it bans by default are all classes of Sets and Maps. Most times it doesn't matter the exact instance you are using, they all extend the same interfaces.

There are checks enforce looser types like when passing a parameter to a method. If you specify HashMap as the parameter and the user only has a Map or LinkedHashMap, it requires them to go through the trouble of redoing the instance just to be able to use the method. Being loose and just accept a Map allows more instances the method can be used without workarounds. Of course, the method has to be able to support all those instances and not really depend on them, but that would be reviewed per instance as this check will bring them to light.

To ban usage of some deprecated class in a codebase
To ban usage of some library class in a codebase

I think ImportControl would be better for these as I mentioned above. You can still instantiate the class with IllegalType.

However, with permits, we would not have to worry about case (2) above, since we can safely assume that we have control over any class that we permit.

Agree, we can't be creating a class and have a library be using that class at the same time. One has to come first, the library or the project we are in.

=====

I am actually the opposite of @nrmancuso .

I think we shouldn't violate these. We aren't declaring this as a type of something. We are just saying a banned type is allowed to extend the sealed class. We won't ban the instantiation of the banned type as shown above. It just can't be used as the declared type. In this scenario, this means users should be doing A a = new IllegalType2() instead of IllegalType2 a = new IllegalType2().

If we were to violate things, it seems to make more sense to violate the declaration of the banned class (final class IllegalType2) which we don't do as shown in the first post. You are trying to say it is fully banned and not even be declared in the first place.

https://docs.oracle.com/javase/specs/jls/se22/html/jls-8.html#jls-8.1.6 - permits
https://docs.oracle.com/javase/specs/jls/se22/html/jls-15.html#jls-15.9 - new
Both areas are basically declared as types in the JLS, so it won't really help this discussion.

I am leaning towards not approving this, but I am not against it being approved. I think if we did approve this, we definitely need a way to enable/disable this since we are not 100% sure on this. This may depend more on what the user is implying by specifying a type that appears in this situation.

We could also get input from a 3rd person.

[nrmancuso]

Let's allow this one to simmer for awhile, and let users get more familiar with this syntax and develop some opinions about it. If there is no clear cut path forward now, let's not push it.