owlcs / owlapi

OWL API main repository
830 stars 315 forks source link

Consider raising Java version requirement to 17 or 21 to allow for algebraic data types #1101

Open julesjacobsen opened 1 year ago

julesjacobsen commented 1 year ago

Hi, I know this is probably a long shot given the downstream dependencies but as you're probably aware Java 17 has been released for at least a year now and is the latest LTS release (soon to be superseded by 21). However, the benefits it brings to the OWLAPI are sealed classes and pattern matching for switch which allows some much cleaner code both internally and for consumers.

Maybe for owlapi version 6 or 7?

For example the OWLAnnotationValue would benefit from being sealed

/**
 * A marker interface for annotation values, which can either be an IRI (URI), Literal or Anonymous
 * Individual, with visitor methods.
 * 
 * @see org.semanticweb.owlapi.model.IRI
 * @see org.semanticweb.owlapi.model.OWLLiteral
 * @see org.semanticweb.owlapi.model.OWLAnonymousIndividual
 * @author Matthew Horridge, The University of Manchester, Information Management Group
 * @since 3.0.0
 */
public interface OWLAnnotationValue extends OWLAnnotationObject, OWLPrimitive {

    /**
     * @param visitor visitor to accept
     */
    void accept(@Nonnull OWLAnnotationValueVisitor visitor);

    /**
     * @param visitor visitor to accept
     * @param <O> visitor return type
     * @return visitor value
     */
    @Nonnull
    <O> O accept(@Nonnull OWLAnnotationValueVisitorEx<O> visitor);

    /**
     * @return if the value is a literal, return an optional containing it. Return Optional.absent
     *         otherwise.
     */
    @Nonnull
    Optional<OWLLiteral> asLiteral();

    /**
     * @return if the value is an IRI, return an optional containing it. Return Optional.absent
     *         otherwise.
     */
    @Nonnull
    Optional<IRI> asIRI();

    /**
     * @return if the value is an anonymous, return an optional containing it. Return
     *         Optional.absent otherwise.
     */
    @Nonnull
    Optional<OWLAnonymousIndividual> asAnonymousIndividual();

    /**
     * @return true if the annotation value is a literal
     */
    default boolean isLiteral() {
        return false;
    }
}

would become

/**
 * A marker interface for annotation values, which can either be an IRI (URI), Literal or Anonymous
 * Individual, with visitor methods.
 * 
 * @see org.semanticweb.owlapi.model.IRI
 * @see org.semanticweb.owlapi.model.OWLLiteral
 * @see org.semanticweb.owlapi.model.OWLAnonymousIndividual
 * @author Matthew Horridge, The University of Manchester, Information Management Group
 * @since 3.0.0
 */
public sealed interface OWLAnnotationValue permits IRI, OWLLiteral, OWLAnonymousIndividual extends OWLAnnotationObject, OWLPrimitive {

}

Where did the visitor pattern accept methods go? This isn't needed with sealed classes and pattern matching for switch. See NipaFX's nice explanation. Also no need to read the comments about what interfaces are permitted.

Similarly the OWLAnnotationSubject

/**
 * A marker interface for annotation subjects, which can either be IRIs or
 * anonymous individuals, with visitor methods.
 * 
 * @author Matthew Horridge, The University of Manchester, Information
 *         Management Group
 * @since 3.0.0
 */
public interface OWLAnnotationSubject extends OWLAnnotationObject, OWLPrimitive {

becomes sealed and permits IRI and OWLAnonymousIndividual

/**
 * A marker interface for annotation subjects, which can either be IRIs or
 * anonymous individuals, with visitor methods.
 * 
 * @author Matthew Horridge, The University of Manchester, Information
 *         Management Group
 * @since 3.0.0
 */
public sealed interface OWLAnnotationSubject permits IRI, OWLAnonymousIndividual extends OWLAnnotationObject, OWLPrimitive {

and so would OWLAnnotationAxiom (would permits OWLAnnotationAssertionAxiom, OWLAnnotationPropertyDomainAxiom, OWLAnnotationPropertyRangeAxiom , OWLSubAnnotationPropertyOfAxiom) which would allow current code like this (taken from obographs):

if (ax instanceof OWLAnnotationAssertionAxiom) {
    OWLAnnotationAssertionAxiom aaa = (OWLAnnotationAssertionAxiom) ax;
    OWLAnnotationProperty p = aaa.getProperty();
    OWLAnnotationSubject s = aaa.getSubject();

    // non-blank nodes
    if (s instanceof IRI) {
        String subj = getNodeId((IRI) s);

        OWLAnnotationValue v = aaa.getValue();

       /// lots of other if tests
     else {
        String val;
        if (v instanceof IRI) {
            val = ((IRI) v).toString();
        } else if (v instanceof OWLLiteral) {
            val = ((OWLLiteral) v).getLiteral();
        } else if (v instanceof OWLAnonymousIndividual) {
            val = ((OWLAnonymousIndividual) v).getID().toString();
        } else {
            val = "";
        }

        BasicPropertyValue basicPropertyValue = new BasicPropertyValue.Builder()
                .pred(getPropertyId(p))
                .val(val)
                .build();

        oboGraphBuilder.addNodeBasicPropertyValue(subj, basicPropertyValue);
    }  else {
        // subject is anonymous
        oboGraphBuilder.addUntranslatedAxiom(aaa);
    }
}

to become a lot less noisy and more succinct:

// 'new' Java 16 - Pattern Matching for instanceof: https://openjdk.org/jeps/394
if (ax instanceof OWLAnnotationAssertionAxiom aaa) {
    OWLAnnotationProperty p = aaa.getProperty();
    OWLAnnotationSubject s = aaa.getSubject();

    // non-blank nodes, switch on sealed class OWLAnnotationSubject allows for compiler-checked cases instead of
    // unchecked if ... else
    // OK, maybe in Java 21 the next LTS release - Pattern Matching for switch: https://openjdk.org/jeps/441 
    switch (s) {
        case IRI sIRI -> {
                String subj = getNodeId(sIRI);

                OWLAnnotationValue v = aaa.getValue();

                /// lots of other if tests
                String val = switch (v) {
                        case IRI iri -> iri.toString();
                        case OWLLiteral owlLiteral -> owlLiteral.getLiteral();
                        case OWLAnonymousIndividual anon -> anon.getID().toString();
                } // no need for guard or default as these are the only allowed options for the sealed interface 
                // and the compiler will check this.

                BasicPropertyValue basicPropertyValue = new BasicPropertyValue.Builder()
                        .pred(getPropertyId(p))
                        .val(val)
                        .build();

                oboGraphBuilder.addNodeBasicPropertyValue(subj, basicPropertyValue);
        };
        case OWLAnonymousIndividual anon -> oboGraphBuilder.addUntranslatedAxiom(aaa);
    }
}
ignazio1977 commented 1 year ago

@julesjacobsen Indeed, version 6 will require Java 17. I've not started work on it yet, but that's the last important change before a release candidate for it. Version 5.5.0 requires Java 11 now, which is a first step up.

julesjacobsen commented 1 year ago

Glad to hear this is on the cards. It's unfortunate that pattern matching for switch isn't final in 17, but I'm guessing you'll be targeting a post-17 LTS (Java 21?) for owlapi version 7 where this could all come together?

ignazio1977 commented 1 year ago

That's more or less the plan, unless the version 6 release candidate ends up coming out when V21 becomes available.

jamesamcl commented 1 year ago

I can't actually get OWLAPI working with Java 17:

java.lang.reflect.InaccessibleObjectException: Unable to make protected final java.lang.Class java.lang.ClassLoader.defineClass(java.lang.String,byte[],int,int,java.security.ProtectionDomain) throws java.lang.ClassFormatError accessible: module java.base does not "opens java.lang" to unnamed module @47f37ef1
        at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:354) ~[na:na]
        at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297) ~[na:na]
        at java.base/java.lang.reflect.Method.checkCanSetAccessible(Method.java:199) ~[na:na]
        at java.base/java.lang.reflect.Method.setAccessible(Method.java:193) ~[na:na]
        at com.google.inject.internal.cglib.core.$ReflectUtils$1.run(ReflectUtils.java:52) ~[guice-4.1.0.jar!/:na]
        at java.base/java.security.AccessController.doPrivileged(AccessController.java:318) ~[na:na]
        at com.google.inject.internal.cglib.core.$ReflectUtils.<clinit>(ReflectUtils.java:42) ~[guice-4.1.0.jar!/:na]
        at com.google.inject.internal.cglib.reflect.$FastClass$Generator.getProtectionDomain(FastClass.java:73) ~[guice-4.1.0.jar!/:na]
        at com.google.inject.internal.cglib.core.$AbstractClassGenerator.create(AbstractClassGenerator.java:206) ~[guice-4.1.0.jar!/:na]
        at com.google.inject.internal.cglib.reflect.$FastClass$Generator.create(FastClass.java:65) ~[guice-4.1.0.jar!/:na]
        at com.google.inject.internal.BytecodeGen.newFastClassForMember(BytecodeGen.java:252) ~[guice-4.1.0.jar!/:na]
        at com.google.inject.internal.BytecodeGen.newFastClassForMember(BytecodeGen.java:203) ~[guice-4.1.0.jar!/:na]
        at com.google.inject.internal.ProviderMethod.create(ProviderMethod.java:69) ~[guice-4.1.0.jar!/:na]
        at com.google.inject.internal.ProviderMethodsModule.createProviderMethod(ProviderMethodsModule.java:275) ~[guice-4.1.0.jar!/:na]
        at com.google.inject.internal.ProviderMethodsModule.getProviderMethods(ProviderMethodsModule.java:144) ~[guice-4.1.0.jar!/:na]
        at com.google.inject.internal.ProviderMethodsModule.configure(ProviderMethodsModule.java:123) ~[guice-4.1.0.jar!/:na]
        at com.google.inject.spi.Elements$RecordingBinder.install(Elements.java:340) ~[guice-4.1.0.jar!/:na]
        at com.google.inject.spi.Elements$RecordingBinder.install(Elements.java:349) ~[guice-4.1.0.jar!/:na]
        at com.google.inject.spi.Elements.getElements(Elements.java:110) ~[guice-4.1.0.jar!/:na]
        at com.google.inject.internal.InjectorShell$Builder.build(InjectorShell.java:138) ~[guice-4.1.0.jar!/:na]
        at com.google.inject.internal.InternalInjectorCreator.build(InternalInjectorCreator.java:104) ~[guice-4.1.0.jar!/:na]
        at com.google.inject.Guice.createInjector(Guice.java:99) ~[guice-4.1.0.jar!/:na]
        at com.google.inject.Guice.createInjector(Guice.java:73) ~[guice-4.1.0.jar!/:na]
        at com.google.inject.Guice.createInjector(Guice.java:62) ~[guice-4.1.0.jar!/:na]
        at org.semanticweb.owlapi.apibinding.OWLManager.createInjector(OWLManager.java:104) ~[owlapi-distribution-5.1.0.jar!/:5.1.0.2017-03-29T23:31:42Z]
        at org.semanticweb.owlapi.apibinding.OWLManager.createOWLOntologyManager(OWLManager.java:44) ~[owlapi-distribution-5.1.0.jar!/:5.1.0.2017-03-29T23:31:42Z]
ignazio1977 commented 1 year ago

@udp Version 5.1.0 was still using Guice, that's not been the case for quite a while. I've not tried running version 5.1.0 with Java newer than 8. Version 5.5.0 is built with Java 11, including tests that exercise this code path. Some of the most recent 5.1.x versions should work as well.