Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nullness Type System for key.core #3470

Draft
wants to merge 19 commits into
base: main
Choose a base branch
from
Draft

Nullness Type System for key.core #3470

wants to merge 19 commits into from

Conversation

wadoon
Copy link
Member

@wadoon wadoon commented May 3, 2024

More nullness type system checks. Now for key.core.

I want first to merge #3399 into KeY to avoid double work.

@wadoon wadoon requested a review from flo2702 May 3, 2024 15:57
@wadoon wadoon self-assigned this May 3, 2024
@wadoon wadoon added the 🛠 Maintenance Code quality and related things w/o functional changes label May 3, 2024
@wadoon wadoon added this to the v2.16.0 milestone May 3, 2024
wadoon and others added 13 commits May 9, 2024 23:58
* refs/heads/main:
  reformat with spotless
  typo
  Unify type annotation notation
  Apply spotless
  Remove unnecessary warnings and serialization
  fix error in the legacy compat part of the proof obligation loading
  revert some changes of Mattias in the Configuration
  fix compile error and reformat
  Configuration: correcting typos, making implementation consistent
  address reviewers comments
  reformat witn spotless
  add fallback and javadoc
  translate WD obligation loader
  fix Lemma Proof Obligations
  migration of existing \proofObligation to the new syntax
  migration script for \proofObligation
  spotless + compiler error
  Renovating PO loader

# Conflicts:
#	key.core/src/main/java/de/uka/ilkd/key/settings/Configuration.java
#	key.core/src/main/java/de/uka/ilkd/key/util/MiscTools.java
# By Florian Lanzinger (25) and others
# Via GitHub (14) and others
* main: (69 commits)
  typo
  Unify type annotation notation
  Apply spotless
  Remove unnecessary warnings and serialization
  key.ncore done
  configure key.ncore
  fix null values
  eisop in ncore
  Fix formatting
  Fix more NoSuchElementExceptions
  Fix NoSuchElementException in JavaInfo
  Remove redundant nullness checks and fix test cases
  Fix proof script
  #equals must allow null values
  jspecify was missing in the compile classpath of tests
  Code style
  Revert JavaRedux Object
  Test case
  Fix merge issues
  reformat files
  ...

# Conflicts:
#	key.core/src/main/java/de/uka/ilkd/key/java/ParseExceptionInFile.java
#	key.core/src/main/java/de/uka/ilkd/key/java/PosConvertException.java
#	key.core/src/main/java/de/uka/ilkd/key/java/Recoder2KeY.java
#	key.core/src/main/java/de/uka/ilkd/key/java/ast/expression/operator/Subtype.java
#	key.core/src/main/java/de/uka/ilkd/key/java/recoderext/RecoderModelTransformer.java
#	key.core/src/main/java/de/uka/ilkd/key/macros/scripts/ScriptException.java
#	key.core/src/main/java/de/uka/ilkd/key/nparser/builder/AbstractBuilder.java
#	key.core/src/main/java/de/uka/ilkd/key/nparser/builder/ExpressionBuilder.java
#	key.core/src/main/java/de/uka/ilkd/key/pp/PrettyPrinter.java
#	key.core/src/main/java/de/uka/ilkd/key/proof/Proof.java
#	key.core/src/main/java/de/uka/ilkd/key/proof/SVInstantiationExceptionWithPosition.java
#	key.core/src/main/java/de/uka/ilkd/key/proof/io/KeYFile.java
#	key.core/src/main/java/de/uka/ilkd/key/speclang/njml/Translator.java
#	key.core/src/main/java/de/uka/ilkd/key/speclang/translation/SLTranslationException.java
#	key.core/src/main/java/de/uka/ilkd/key/util/ExceptionTools.java
#	key.core/src/main/java/de/uka/ilkd/key/util/RecognitionException.java
#	key.core/src/main/java/de/uka/ilkd/key/util/parsing/BuildingException.java
#	key.core/src/main/java/de/uka/ilkd/key/util/parsing/LocatableException.java
#	key.core/src/main/java/recoder/service/KeYCrossReferenceSourceInfo.java
#	key.util/src/main/java/org/key_project/util/ExtList.java
#	key.util/src/main/java/org/key_project/util/collection/ImmutableList.java
#	settings.gradle
# By Florian Lanzinger (25) and others
# Via GitHub (14) and others
* main: (69 commits)
  typo
  Unify type annotation notation
  Apply spotless
  Remove unnecessary warnings and serialization
  key.ncore done
  configure key.ncore
  fix null values
  eisop in ncore
  Fix formatting
  Fix more NoSuchElementExceptions
  Fix NoSuchElementException in JavaInfo
  Remove redundant nullness checks and fix test cases
  Fix proof script
  #equals must allow null values
  jspecify was missing in the compile classpath of tests
  Code style
  Revert JavaRedux Object
  Test case
  Fix merge issues
  reformat files
  ...

# Conflicts:
#	key.core/src/main/java/de/uka/ilkd/key/java/ParseExceptionInFile.java
#	key.core/src/main/java/de/uka/ilkd/key/java/PosConvertException.java
#	key.core/src/main/java/de/uka/ilkd/key/java/Recoder2KeY.java
#	key.core/src/main/java/de/uka/ilkd/key/java/ast/expression/operator/Subtype.java
#	key.core/src/main/java/de/uka/ilkd/key/java/recoderext/RecoderModelTransformer.java
#	key.core/src/main/java/de/uka/ilkd/key/macros/scripts/ScriptException.java
#	key.core/src/main/java/de/uka/ilkd/key/nparser/builder/AbstractBuilder.java
#	key.core/src/main/java/de/uka/ilkd/key/nparser/builder/ExpressionBuilder.java
#	key.core/src/main/java/de/uka/ilkd/key/pp/PrettyPrinter.java
#	key.core/src/main/java/de/uka/ilkd/key/proof/Proof.java
#	key.core/src/main/java/de/uka/ilkd/key/proof/SVInstantiationExceptionWithPosition.java
#	key.core/src/main/java/de/uka/ilkd/key/proof/io/KeYFile.java
#	key.core/src/main/java/de/uka/ilkd/key/speclang/njml/Translator.java
#	key.core/src/main/java/de/uka/ilkd/key/speclang/translation/SLTranslationException.java
#	key.core/src/main/java/de/uka/ilkd/key/util/ExceptionTools.java
#	key.core/src/main/java/de/uka/ilkd/key/util/RecognitionException.java
#	key.core/src/main/java/de/uka/ilkd/key/util/parsing/BuildingException.java
#	key.core/src/main/java/de/uka/ilkd/key/util/parsing/LocatableException.java
#	key.core/src/main/java/recoder/service/KeYCrossReferenceSourceInfo.java
#	key.util/src/main/java/org/key_project/util/ExtList.java
#	key.util/src/main/java/org/key_project/util/collection/ImmutableList.java
#	settings.gradle
* refs/heads/main: (26 commits)
  Fix comment
  Fix checkstyle workflow
  Fix checkstyle workflow
  Fix merge conflicts & spotless
  Remove todo
  Spotless
  Fix? resolving error
  Move ParsableVariable to ncore
  Spotless
  Spotless
  Rename AbstractSV to OperatorSV
  Beautified code
  Fix settings test for SE
  Spotless fixes
  Fix taclet prefix check when parsing
  Fix taclet equality test
  Fix parsing of variable conditions
  Fix errors resulting from changing ParseableVar
  Delete Legacy Matcher and adapt VM matcher for new Modality operator
  Fix errors after changing ParsableVars
  ...

# Conflicts:
#	key.core/src/main/java/de/uka/ilkd/key/util/MiscTools.java
* also fix some encoding in recorder/src files
* main: (77 commits)
  Update key.core/src/main/java/de/uka/ilkd/key/nparser/builder/TacletPBuilder.java
  spotless
  update oracle for taclet equality test
  change gradle github action to new syntax
  adding comments to jml spec factory default contracts
  repair soundness of assignment2UpdateRules with checked overflows
  spotless
  EQ version of seqSwapPreservesSeqPerm + proof
  added rule for sequences: swap preserves perm
  Changed types in replacement map for WD taclets, since PR #3436 made casting TermSV to ProgramVariable not applicable
  spotlessing ...
  making RuleCommand work if already fully instantiated
  RuleCommand can now deal with rules that have schema variables for logical variables.
  Fix loading of taclet proof obligations (issue #3477) * This commit fixes an NPE when loading * This commit fixes missing or inconsistent selection of loaded proof   obligation
  Code clean up (remove unused method)
  Fix loading of closed proofs (GUI threw error)
  Fix and test goToNext()
  Fix goToNextSibling() (thx Tobias)
  Format
  Add comments and next() method
  ...
* refs/heads/weigl/codequality:
  reenable sonarqube, disable the crappy things
* origin/main: (77 commits)
  Bump the gradle-deps group across 1 directory with 5 updates
  Also depend on `checker-qual` artifact
  Bump the github-actions-deps group with 5 updates
  Minor refactoring to remove duplicate code
  fixing the broken automode
  resolve reviewer requests
  applied formatting style
  disable automatic formatting of Java code blocks in comments/JavaDoc
  removed formatter version lock and added new keys (via new styleMerge tool)
  added small utility to merge xml formatter style files
  add javadoc
  add javadoc
  reformat fix finalize() deprecation
  fix error in expecting proof script if there is none
  fix compile errors due to merging
  forgot ProofScriptEntry
  fix compile errors
  Removal of the Triple class
  Removal of Quadruple.java
  fix auto merger in github workflow
  ...

# Conflicts:
#	.github/workflows/code_quality.yml
#	.github/workflows/sonarqube.yml
#	build.gradle
#	key.util/src/main/java/org/key_project/util/collection/ImmutableMap.java
@KeYProject KeYProject deleted a comment from sonarqubecloud bot Jan 23, 2025
@KeYProject KeYProject deleted a comment from sonarqubecloud bot Jan 23, 2025
@wadoon
Copy link
Member Author

wadoon commented Jan 23, 2025

I have pushed further and activated Nullness checks for more packages but have not finished everything. Annoying are the situations where the InitilizationChecker hits:

class A {
  public A() { ... 
    setFoo(2);
  }
  void setFoo(int i){...}
}

The complain is that setFoo is called with @Unintialized A this but expects @Initialized A this.

@wmdietl @flo2702
Is there any trick to get pass this message? For example, an

assert this instanceof @Initialized A;

would be fine.

Inside KeY, we have many of these moments. It is also not a bad code design and is hard to avoid. Also adding the right annotation leads to ugly code:

class A {
  public A() { ...; setFoo(2);  }
  void setFoo(@Uninitialized A this, int i){...}
}

and this needs to be added to all transitive called methods.

@wmdietl
Copy link
Contributor

wmdietl commented Jan 23, 2025

The warning from the Initialization Checker is correct.
Class A isn't final, so there could be the following code:

class A {
  public A() { ... 
    setFoo(2);
  }
  void setFoo(int i){...}
}

class B extends A {
  Object o; // non-null by default

  @Override
  void setFoo(int i) {
    foo = i + o.hashCode(); 
  }
}

This code would lead to an NPE, as setFoo is called before B.o has been initialized.
B.setFoo is correctly implemented, as the receiver of setFoo promises a fully initialized receiver.

The right thing to do is to document this by setting the receiver type of setFoo.
The wrong thing to do is to suppress the warning on the call in the constructor of A (and at least add a comment to setFoo that it will be invoked on an under-initialization receiver).

Addition: you could use UnknownInitialization(A.class) A this as the receiver type of setFoo to express that the receiver is initialized up to and including class A.

@wadoon
Copy link
Member Author

wadoon commented Jan 23, 2025

@wmdietl I saw that this error is correct. However, I want to avoid the clutter of receiver types in the source code.

@wmdietl
Copy link
Contributor

wmdietl commented Jan 23, 2025

@wadoon If this only happens in a few constructors, simply suppress the warning there.
If you never care about warnings from the Initialization Checker, pass -AassumeInitialized to turn it off.
You might be able to get something in-between by passing -AsuppressWarnings= with the error key from the call... https://eisop.github.io/cf/manual/manual.html#suppresswarnings-command-line

Do you see any other option that we could add to the framework?

@wadoon
Copy link
Member Author

wadoon commented Jan 23, 2025

For the first, I stick to -AassumeInitialized.

This pattern of calling own methods in constructor is rather common in UI code, e.g., AbstractAction in Swing.

Do you see any other option that we could add to the framework?

I am thinking about, why is this so easy in Kotlin, and in Java so complicated.

My first thought was that a simpler annotation would be fine. At least lower the clutter.

typealias @ActionInit = `@UnknownInitialization(javax.swing.AbstractAction.class) KeYAction this`;
public class KeyAction { @ActionInit public KeyAction setName(...) {...} }

My second thought was, why am I not able to stop the "under-initialization phase" by calling this(..); or getting an UnknownInitialization(A.class) A this by calling super();

Here is the excerpt on how we typically build our Swing Actions:

abstract class javax.swing.AbstractAction  implements ActionListener {
   private Map<> attrib = new HashMap<>();
   public void putValue(Object key, Object value);
}

class KeyAction extends AbstractAction {   
    void setName(String name) { putValue(NAME, name);} 
}

class AboutDialogAction extends KeyAction {
    public AboutDialog() { 
        super(); // normally implicit
        setName("About..."); // <--- should be possible KeyAction is constructed.
    }
}

Of course, there might be loopholes (super() calls an overridden method) for certain scenarios this should be valid, especially when a class is "encapsulated under inheritance" iff. I could use the parent class also via delegation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🛠 Maintenance Code quality and related things w/o functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants