Skip to content

Commit

Permalink
Reflow comments
Browse files Browse the repository at this point in the history
  • Loading branch information
mernst authored Apr 1, 2021
1 parent 2a22089 commit b79ec90
Show file tree
Hide file tree
Showing 134 changed files with 723 additions and 991 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,9 @@
import org.checkerframework.framework.qual.InheritedAnnotation;
import org.checkerframework.framework.qual.PostconditionAnnotation;

// TODO: In a fix for https://tinyurl.com/cfissue/1917, add the text:
// Every prefix expression is also non-null; for example, {@code
// @EnsuresNonNull(expression="a.b.c")} implies that both {@code a.b} and {@code a.b.c} are
// non-null.
// TODO: In a fix for https://tinyurl.com/cfissue/1917, add the text: Every prefix expression is
// also non-null; for example, {@code @EnsuresNonNull(expression="a.b.c")} implies that both {@code
// a.b} and {@code a.b.c} are non-null.
/**
* Indicates that the value expressions are non-null just after a method call, if the method
* terminates successfully.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,9 @@
import org.checkerframework.framework.qual.ConditionalPostconditionAnnotation;
import org.checkerframework.framework.qual.InheritedAnnotation;

// TODO: In a fix for https://tinyurl.com/cfissue/1917, add the text:
// Every prefix expression is also non-null; for example,
// {@code @EnsuresNonNullIf(expression="a.b.c", results=true)} implies that both {@code a.b} and
// {@code a.b.c} are non-null if the method returns {@code true}.
// TODO: In a fix for https://tinyurl.com/cfissue/1917, add the text: Every prefix expression is
// also non-null; for example, {@code @EnsuresNonNullIf(expression="a.b.c", results=true)} implies
// that both {@code a.b} and {@code a.b.c} are non-null if the method returns {@code true}.
/**
* Indicates that the given expressions are non-null, if the method returns the given result (either
* true or false).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,9 @@
import java.lang.annotation.Target;
import org.checkerframework.framework.qual.PreconditionAnnotation;

// TODO: In a fix for https://tinyurl.com/cfissue/1917, add the text:
// Every prefix expression must also be non-null; for example, {@code
// @RequiresNonNull(expression="a.b.c")} implies that both {@code a.b} and {@code a.b.c} must be
// non-null.
// TODO: In a fix for https://tinyurl.com/cfissue/1917, add the text: Every prefix expression must
// also be non-null; for example, {@code @RequiresNonNull(expression="a.b.c")} implies that both
// {@code a.b} and {@code a.b.c} must be non-null.
/**
* Indicates a method precondition: the method expects the specified expressions to be non-null when
* the annotated method is invoked.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@
* @checker_framework.manual #aliasing-checker Aliasing Checker
*/

// This is a type qualifier because of a checker framework limitation (Issue 383), but its
// hierarchy is ignored. Once the stub parser gets updated to read non-type-qualifiers
// annotations on stub files, this annotation won't be a type qualifier anymore.
// This is a type qualifier because of a checker framework limitation (Issue 383), but its hierarchy
// is ignored. Once the stub parser gets updated to read non-type-qualifiers annotations on stub
// files, this annotation won't be a type qualifier anymore.

@Documented
@Retention(RetentionPolicy.RUNTIME)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@
* @checker_framework.manual #aliasing-checker Aliasing Checker
*/

// This is a type qualifier because of a checker framework limitation (Issue 383), but its
// hierarchy is ignored. Once the stub parser gets updated to read non-type-qualifiers
// annotations on stub files, this annotation won't be a type qualifier anymore.
// This is a type qualifier because of a checker framework limitation (Issue 383), but its hierarchy
// is ignored. Once the stub parser gets updated to read non-type-qualifiers annotations on stub
// files, this annotation won't be a type qualifier anymore.

@Documented
@Retention(RetentionPolicy.RUNTIME)
Expand Down
2 changes: 1 addition & 1 deletion checker/jtreg/nullness/Issue347-con.out
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
Issue347.java:33:5: compiler.err.proc.messager: [dereference.of.nullable] dereference of possibly-null reference nble
Issue347.java:32:5: compiler.err.proc.messager: [dereference.of.nullable] dereference of possibly-null reference nble
1 error
5 changes: 2 additions & 3 deletions checker/jtreg/nullness/Issue347.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,8 @@ void testMono() {
if (mono == null) {
return;
}
// The object referenced by mono might change, but
// it can't become null again, even in concurrent
// semantics.
// The object referenced by mono might change, but it can't become null again, even in
// concurrent semantics.
mono.toString();
}

Expand Down
3 changes: 1 addition & 2 deletions checker/jtreg/nullness/inheritDeclAnnoPersist/Extends.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,7 @@ public String m2() {
}

// Issue 342
// We do not want that behavior with related annotations. @Pure should
// override @SideEffectFree.
// We do not want that behavior with related annotations. @Pure should override @SideEffectFree.
@ADescriptions({
@ADescription(annotation = "org/checkerframework/dataflow/qual/Pure"),
@ADescription(annotation = "org/checkerframework/dataflow/qual/SideEffectFree")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,10 +215,9 @@ filterTree, collectionsSingletonList, getProcessingEnv())) {
// Proceed leftward (toward the receiver) in a fluent call sequence.
filterTree = TreeUtils.getReceiverTree(filterTreeAsMethodInvocation.getMethodSelect());
}
// The loop has reached the beginning of a fluent sequence of method calls.
// If the ultimate receiver at the beginning of that fluent sequence is a
// call to the Filter() constructor, then use the first argument to the Filter
// constructor, which is the name of the filter.
// The loop has reached the beginning of a fluent sequence of method calls. If the ultimate
// receiver at the beginning of that fluent sequence is a call to the Filter() constructor, then
// use the first argument to the Filter constructor, which is the name of the filter.
if (filterTree == null) {
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -246,10 +246,9 @@ private static String autoValuePropToBuilderSetterName(
}
}

// Could not find a corresponding setter. This is likely because an AutoValue Extension is
// in use. See https://github.com/kelloggm/object-construction-checker/issues/110 .
// For now we return null, but once that bug is fixed, this should be changed to an
// assertion failure.
// Could not find a corresponding setter. This is likely because an AutoValue Extension is in
// use. See https://github.com/kelloggm/object-construction-checker/issues/110 . For now we
// return null, but once that bug is fixed, this should be changed to an assertion failure.
return null;
}

Expand Down Expand Up @@ -277,8 +276,7 @@ private List<String> getAutoValueRequiredProperties(
*/
private boolean isAutoValueRequiredProperty(Element member, Set<String> avBuilderSetterNames) {
String name = member.getSimpleName().toString();
// Ignore java.lang.Object overrides, constructors, and toBuilder methods in AutoValue
// classes.
// Ignore java.lang.Object overrides, constructors, and toBuilder methods in AutoValue classes.
// Strictly speaking, this code should check return types, etc. to handle strange
// overloads and other corner cases. They seem unlikely enough that we are skipping for now.
if (ArraysPlume.indexOf(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,9 @@ private List<String> getLombokRequiredProperties(final Element lombokClassElemen
List<String> defaultedPropertyNames = new ArrayList<>();
for (Element member : lombokClassElement.getEnclosedElements()) {
if (member.getKind() == ElementKind.FIELD) {
// Lombok never generates non-null fields with initializers in builders, unless
// the field is annotated with @Default or @Singular, which are handled elsewhere.
// So, this code doesn't need to consider whether the field has or does not have
// initializers.
// Lombok never generates non-null fields with initializers in builders, unless the field is
// annotated with @Default or @Singular, which are handled elsewhere. So, this code doesn't
// need to consider whether the field has or does not have initializers.
for (AnnotationMirror anm :
atypeFactory.getElementUtils().getAllAnnotationMirrors(member)) {
if (NONNULL_ANNOTATIONS.contains(AnnotationUtils.annotationName(anm))) {
Expand All @@ -165,13 +164,12 @@ private List<String> getLombokRequiredProperties(final Element lombokClassElemen
defaultedPropertyNames.add(propName);
}
} else if (member.getKind().isClass() && member.toString().endsWith("Builder")) {
// Note that the test above will fail to catch builders generated by Lombok
// that have custom names using the builderClassName attribute. TODO: find a way
// to handle such builders too.
// Note that the test above will fail to catch builders generated by Lombok that have custom
// names using the builderClassName attribute. TODO: find a way to handle such builders too.

// If a field bar has an @Singular annotation, Lombok always generates a method
// called clearBar in the builder class itself. Therefore, search the builder for
// such a method, and extract the appropriate property name to treat as defaulted.
// If a field bar has an @Singular annotation, Lombok always generates a method called
// clearBar in the builder class itself. Therefore, search the builder for such a method,
// and extract the appropriate property name to treat as defaulted.
for (Element builderMember : member.getEnclosedElements()) {
if (builderMember.getKind() == ElementKind.METHOD
&& ElementUtils.hasAnnotation(builderMember, "lombok.Generated")) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,9 @@ protected Set<? extends AnnotationMirror> getExceptionParameterLowerBoundAnnotat
@Override
public boolean isValidUse(
AnnotatedDeclaredType declarationType, AnnotatedDeclaredType useType, Tree tree) {
// The checker calls this method to compare the annotation used in a
// type to the modifier it adds to the class declaration. As our default
// modifier is FenumBottom, this results in an error when a non-subtype
// is used. Can we use FenumTop as default instead?
// The checker calls this method to compare the annotation used in a type to the modifier it
// adds to the class declaration. As our default modifier is FenumBottom, this results in an
// error when a non-subtype is used. Can we use FenumTop as default instead?
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -191,9 +191,8 @@ public boolean isFormatMethodCall(MethodInvocationTree node, AnnotatedTypeFactor
ExecutableElement methodElement = TreeUtils.elementFromUse(invocationTree);
int formatStringIndex = FormatterVisitor.formatStringIndex(methodElement);
if (formatStringIndex == -1) {
// Reporting the error is redundant if the method was declared in source code, because
// the visitor will have reported it; but it is necessary if the method was declared in
// byte code.
// Reporting the error is redundant if the method was declared in source code, because the
// visitor will have reported it; but it is necessary if the method was declared in byte code.
atypeFactory
.getChecker()
.reportError(invocationTree, "format.method.invalid", methodElement.getSimpleName());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,7 @@ public Void visitMethodInvocation(MethodInvocationTree node, Void p) {
int argl = argTypes.length;
int formatl = formatCats.length;
if (argl < formatl) {
// For assignments, format.missing.arguments is issued
// from commonAssignmentCheck.
// For assignments, format.missing.arguments is issued from commonAssignmentCheck.
// II.1
ftu.failure(invc, "format.missing.arguments", formatl, argl);
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,7 @@ public boolean isUIType(TypeElement cls) {
}

// Anon inner classes should not inherit the package annotation, since
// they're so often used for closures to run async on background
// threads.
// they're so often used for closures to run async on background threads.
if (isAnonymousType(cls)) {
// However, we need to look into Anonymous class effect inference
if (uiAnonClasses.contains(cls)) {
Expand Down Expand Up @@ -250,10 +249,9 @@ public Effect getDeclaredEffect(ExecutableElement methodElt) {
return new Effect(UIEffect.class);
}

// Anonymous inner types should just get the effect of the parent by
// default, rather than annotating every instance. Unless it's
// implementing a polymorphic supertype, in which case we still want the
// developer to be explicit.
// Anonymous inner types should just get the effect of the parent by default, rather than
// annotating every instance. Unless it's implementing a polymorphic supertype, in which case we
// still want the developer to be explicit.
if (isAnonymousType(targetClassElt)) {
boolean canInheritParentEffects = true; // Refine this for polymorphic parents
DeclaredType directSuper = (DeclaredType) targetClassElt.getSuperclass();
Expand Down Expand Up @@ -489,9 +487,8 @@ public Effect.EffectRange findInheritedEffectRange(
assert eff.isPoly();
polyOverriden = overriddenMethodElt;
if (isUI) {
// Need to special case an anonymous class with @UI on
// the decl, because "new @UI Runnable {...}" parses as
// @UI on an anon class decl extending Runnable
// Need to special case an anonymous class with @UI on the decl, because "new @UI Runnable
// {...}" parses as @UI on an anon class decl extending Runnable
boolean isAnonInstantiation =
isAnonymousType(declaringType)
&& (fromElement(declaringType).hasAnnotation(UI.class)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,8 @@ protected boolean checkReceiverOverride() {
overriddenType.getUnderlyingType().asElement(), PolyUIType.class)
!= null;
// TODO: How much validation do I need here? Do I need to check that the overridden
// receiver was really @PolyUI and the method is really an @PolyUIEffect? I don't
// think so - we know it's a polymorphic parent type, so all receivers would be
// @PolyUI.
// receiver was really @PolyUI and the method is really an @PolyUIEffect? I don't think so
// - we know it's a polymorphic parent type, so all receivers would be @PolyUI.
// Java would already reject before running type annotation processors if the Java
// types were wrong.
// The *only* extra leeway we want to permit is overriding @PolyUI receiver to
Expand Down Expand Up @@ -359,19 +358,18 @@ public Void visitMethodInvocation(MethodInvocationTree node, Void p) {

@Override
public Void visitMethod(MethodTree node, Void p) {
// TODO: If the type we're in is a polymorphic (over effect qualifiers) type, the receiver
// must be @PolyUI. Otherwise a "non-polymorphic" method of a polymorphic type could be
// called on a UI instance, which then gets a Safe reference to itself (unsound!) that it
// can then pass off elsewhere (dangerous!). So all receivers in methods of a @PolyUIType
// must be @PolyUI.
// TODO: If the type we're in is a polymorphic (over effect qualifiers) type, the receiver must
// be @PolyUI. Otherwise a "non-polymorphic" method of a polymorphic type could be called on a
// UI instance, which then gets a Safe reference to itself (unsound!) that it can then pass off
// elsewhere (dangerous!). So all receivers in methods of a @PolyUIType must be @PolyUI.

// TODO: What do we do then about classes that inherit from a concrete instantiation? If it
// subclasses a Safe instantiation, all is well. If it subclasses a UI instantiation, then
// the receivers should probably be @UI in both new and override methods, so calls to
// polymorphic methods of the parent class will work correctly. In which case for proving
// anything, the qualifier on sublasses of UI instantiations would always have to be
// @UI... Need to write down |- t for this system! And the judgments for method overrides
// and inheritance! Those are actually the hardest part of the system.
// subclasses a Safe instantiation, all is well. If it subclasses a UI instantiation, then the
// receivers should probably be @UI in both new and override methods, so calls to polymorphic
// methods of the parent class will work correctly. In which case for proving anything, the
// qualifier on sublasses of UI instantiations would always have to be @UI... Need to write down
// |- t for this system! And the judgments for method overrides and inheritance! Those are
// actually the hardest part of the system.

ExecutableElement methElt = TreeUtils.elementFromDeclaration(node);
if (debugSpew) {
Expand Down Expand Up @@ -404,8 +402,7 @@ public Void visitMethod(MethodTree node, Void p) {
atypeFactory.findInheritedEffectRange(
((TypeElement) methElt.getEnclosingElement()), methElt, true, node);
// if (targetUIP == null && targetSafeP == null && targetPolyP == null) {
// implicitly annotate this method with the LUB of the effects of the methods it
// overrides
// implicitly annotate this method with the LUB of the effects of the methods it overrides
// atypeFactory.fromElement(methElt).addAnnotation(range != null ? range.min.getAnnot()
// : (isUIType(((TypeElement)methElt.getEnclosingElement())) ? UI.class :
// AlwaysSafe.class));
Expand Down Expand Up @@ -574,7 +571,7 @@ private void scanUp(TreePath path) {
// Push a null method and UI effect onto the stack for static field initialization
// TODO: Figure out if this is safe! For static data, almost certainly,
// but for statically initialized instance fields, I'm assuming those
// are implicitly moved into each constructor, which must then be @UI
// are implicitly moved into each constructor, which must then be @UI.
// currentMethods.addFirst(null);
// effStack.addFirst(new Effect(UIEffect.class));
// super.processClassTree(node);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public TransferResult<CFValue, CFStore> visitMethodInvocation(
// @I18nMakeFormat that will be used to annotate ResourceBundle.getString() so that when the
// getString() method is called, this will check if the given key exist in the translation
// file and annotate the result string with the correct format annotation according to the
// corresponding key's value
// corresponding key's value.
if (tu.isMakeFormatCall(node, atypeFactory)) {
Result<I18nConversionCategory[]> cats = tu.makeFormatCallCategories(node, atypeFactory);
if (cats.value() == null) {
Expand Down
Loading

0 comments on commit b79ec90

Please sign in to comment.