Introduce optimistic default - #1386
Draft
aosen-xiong wants to merge 15 commits into
Draft
Conversation
wmdietl
requested changes
Mar 22, 2026
wmdietl
left a comment
wmdietl
left a comment
Member
There was a problem hiding this comment.
@aosen-xiong @thisisalexandercook We had discussed this PR in the past. Let's go through this and related PRs next week and decide which direction to go.
| * @param annotationScope the element that the conservative default might apply to | ||
| * @return whether the conservative default applies to the given element | ||
| */ | ||
| public boolean applyOptimisticDefaults(Element annotationScope) { |
Member
There was a problem hiding this comment.
Most of this code is just copy-and-paste of applyConservativeDefaults. Can you find a way to share more of the common logic?
| if (applyConservativeDefaults(annotationScope)) { | ||
| for (Default def : uncheckedCodeDefaults) { | ||
| for (Default def : conservativeUncheckedCodeDefaults) { | ||
| if (!typeVarUseDef || def.location != TypeUseLocation.TYPE_VARIABLE_USE) { |
Member
There was a problem hiding this comment.
This also duplicates the logic in the else branch.
Can you first set which defaults to use and then iterate over that set once?
| } | ||
| } | ||
|
|
||
| for (TypeUseLocation loc : CONSERVATIVE_UNCHECKED_DEFAULTS_TOP) { |
Member
There was a problem hiding this comment.
Why iterate over all these sets? Won't most of these conflict with defaults that were already set above?
I think this logic needs some cleaning up.
| */ | ||
| protected void addUncheckedStandardDefaults(QualifierDefaults defs) { | ||
| defs.addUncheckedStandardDefaults(); | ||
| protected void addUncheckedDefaults(QualifierDefaults defs) { |
Member
There was a problem hiding this comment.
Why rename this method? It will still set the standard defaults. What changed?
| * @param kindOfCode source or bytecode | ||
| * @return whether optimistic defaults should be used | ||
| */ | ||
| public boolean useOptimisticDefault(String kindOfCode) { |
| "checkEnclosingExpr", | ||
|
|
||
| // Whether to use optimistic defaults for bytecode and/or source code. | ||
| // The option takes same arguments as "useConservativeDefaultsForUncheckedCode". |
Member
There was a problem hiding this comment.
The ordering of the options should be switched, instead of having a forward reference here.
wmdietl
reviewed
Mar 22, 2026
|
|
||
| The new command-line option `-AuseOptimisticDefaultsForUncheckedCode` takes `source` and `bytecode` argument, similar to | ||
| `-AuseConservativeDefaultsForUnCheckedCode` but apply to optimistic default, that is, Top for method parameter type and | ||
| Bottom for method return and field type. |
Member
There was a problem hiding this comment.
We also need to go through the manual and discuss this new option there.
The branch was 310 commits behind, and master reworked the code it changes. Carry the optimistic defaults forward onto master's design: - eisop#1331 removed QualifierDefaults.isElementAnnotatedForThisChecker. The new applyOptimisticDefaults calls BaseTypeChecker.isElementAnnotatedForThisCheckerOrUpstreamChecker instead, as applyConservativeDefaults now does, and shares its cache. - applyDefaultsElement now applies a memoized, precedence-ordered default list from fusedDefaultsFor rather than looping per call. Its boolean parameter becomes a three-valued DefaultsMode, with a third empty-scope slot and a third identity cache; invalidateFusedDefaults clears all six. - applyOptimisticDefaults mirrors applyConservativeDefaults, including the fast path on the two flags and the isParsingAnnotationFile guard. Without the guard it reaches the checker before the visitor is installed and throws a NullPointerException; without the fast path it costs a stub-file and bytecode test on every defaulted type. - master's bytecode test is atypeFactory.isFromByteCode, not the three-part isElementFromByteCode/declarationFromElement/isFromStubFile check. Keep the existing method names. Renaming addUncheckedCodeDefault and addUncheckedStandardDefaults would break downstream checkers, and was why this branch disabled the JSpecify reference checker in CI; the optimistic variants are added beside them instead, and that CI script is restored. Fix the bytecode branch of applyOptimisticDefaults, which tested the source flag. Regenerate the four expected outputs in checker/jtreg/nullness/onlyannotatedfor: the added @compile directive shifts every line number by one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…icting flags Optimistic defaults put bottom qualifiers where the conservative ones put top. A qualifier can restrict where it may be written with @TargetLocations -- @KeyForBottom and @FBCBottom are not permitted on a RETURN or FIELD -- and defaulting one there made BaseTypeValidator report type.invalid.annotations.on.location on code the user never wrote. Five of the nine expected errors in AnnotatedForWithUseOptimisticDefault.out were that noise. Skip a qualifier/location pair the qualifier prohibits; the hierarchy's other defaults still apply there. The expected output drops to three errors, which match what the test's own comments say case 4 should produce. Defaulting a kind of code both optimistically and conservatively is always a mistake, and previously the conservative defaults silently won. Reject it in SourceChecker.initChecker, where the other option validation lives. An assert would not do: assertions are disabled in a normal javac run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1359.
I disable the JSpecify reference checker CI because the method name are changing. I will enable and make the change at there (if necessary) and enable the CI.
Merge #1304 first.