Adding rule for enforcing final field usage - DoNotMerge until all issues are fixed

eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/EnforceFinalFieldsCheck.java

@@ -0,0 +1,248 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+package com.azure.tools.checkstyle.checks;
+
+import com.puppycrawl.tools.checkstyle.api.AbstractCheck;
+import com.puppycrawl.tools.checkstyle.api.DetailAST;
+import com.puppycrawl.tools.checkstyle.api.TokenTypes;
+import com.puppycrawl.tools.checkstyle.utils.TokenUtil;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * Verify the whenever a field is assigned just once in constructor to be final
+ * Tree traversal will pre-compute and fill 3 private containers:
+ * nonFinalFields: keep an array of non private fields as tokens (to keep line number)
+ * assignmentsFromConstructor: Save a set of string for each field name that gets its value assigned in constructor
+ * assignmentsFromMethods: Save a set of strings for each field name that gets updated in any method
+ *
+ * On finish tree, check what non-final fields get a value only in constructor and nowhere else by looking for
+ * strings inside nonFinalFields AND assignmentsFromConstructor but NOT in assignmentsFromMethods
+ */
+public class EnforceFinalFieldsCheck extends AbstractCheck {
+    private static final String ERROR_SUGGESTION = "You should consider making the field final, " +
+        "or suppressing the warning.";
+    private static final String ERROR_MSG = "Field \"%s\" is only assigned in constructor and it is not final. " +
+        ERROR_SUGGESTION;
+    private static final String ERROR_FIELD_ALONE = "Field \"%s\" is not assigned in constructor or methods." +
+        ERROR_SUGGESTION;
+
+    private List<DetailAST> nonFinalFields;
+    private Set<String> assignmentsFromConstructor;
+    private Set<String> assignmentsFromMethods;
+    private DetailAST scopeParent = null;
+    private Set<String> currentScopeParameterSet = null;
+    private String currentClassName = null;
+
+    @Override
+    public int[] getDefaultTokens() {
+        return getRequiredTokens();
+    }
+
+    @Override
+    public int[] getAcceptableTokens() {
+        return getRequiredTokens();
+    }
+
+    @Override
+    public int[] getRequiredTokens() {
+        return new int[] {
+            TokenTypes.CLASS_DEF,
+            TokenTypes.ASSIGN,
+            TokenTypes.PLUS_ASSIGN,
+            TokenTypes.BAND_ASSIGN,
+            TokenTypes.BOR_ASSIGN,
+            TokenTypes.BSR_ASSIGN,
+            TokenTypes.BXOR_ASSIGN,
+            TokenTypes.DIV_ASSIGN,
+            TokenTypes.MINUS_ASSIGN,
+            TokenTypes.MOD_ASSIGN,
+            TokenTypes.SL_ASSIGN,
+            TokenTypes.SR_ASSIGN,
+            TokenTypes.STAR_ASSIGN,
+            TokenTypes.INC,
+            TokenTypes.POST_INC,
+            TokenTypes.DEC,
+            TokenTypes.POST_DEC,
+            TokenTypes.METHOD_DEF,
+            TokenTypes.CTOR_DEF,
+        };
+    }
+
+    @Override
+    public void beginTree(DetailAST root) {
+        nonFinalFields = new ArrayList<>();
+        assignmentsFromConstructor = new HashSet<>();
+        assignmentsFromMethods = new HashSet<>();
+    }
+
+    @Override
+    public void visitToken(DetailAST token) {
+        switch (token.getType()) {
+            case TokenTypes.CLASS_DEF:
+                this.currentClassName = token.findFirstToken(TokenTypes.IDENT).getText();
+                fillClassFieldDefinitions(token);
+                break;
+            case TokenTypes.ASSIGN:
+            case TokenTypes.PLUS_ASSIGN:
+            case TokenTypes.BAND_ASSIGN:
+            case TokenTypes.BOR_ASSIGN:
+            case TokenTypes.BSR_ASSIGN:
+            case TokenTypes.BXOR_ASSIGN:
+            case TokenTypes.DIV_ASSIGN:
+            case TokenTypes.MINUS_ASSIGN:
+            case TokenTypes.MOD_ASSIGN:
+            case TokenTypes.SL_ASSIGN:
+            case TokenTypes.SR_ASSIGN:
+            case TokenTypes.STAR_ASSIGN:
+            case TokenTypes.INC:
+            case TokenTypes.POST_INC:
+            case TokenTypes.DEC:
+            case TokenTypes.POST_DEC:
+                checkAssignation(token);
+                break;
+            case TokenTypes.METHOD_DEF:
+            case TokenTypes.CTOR_DEF:
+                scopeParent = token;
+            default:
+                // Checkstyle complains if there's no default block in switch
+                break;
+        }
+    }
+
+    @Override
+    public void leaveToken(DetailAST token) {
+        switch (token.getType()) {
+            case TokenTypes.METHOD_DEF:
+            case TokenTypes.CTOR_DEF:
+                scopeParent = null;
+                currentScopeParameterSet = null;
+            default:
+                break;
+        }
+    }
+
+    @Override
+    public void finishTree(DetailAST token) {
+        for (DetailAST field : nonFinalFields) {
+            final String fieldName = field.findFirstToken(TokenTypes.IDENT).getText();
+            if (assignmentsFromConstructor.contains(fieldName) && !assignmentsFromMethods.contains(fieldName)) {
+                log(field, String.format(ERROR_MSG, fieldName));
+            } else if (field.branchContains(TokenTypes.ASSIGN)
+                && !assignmentsFromConstructor.contains(fieldName)
+                && !assignmentsFromMethods.contains(fieldName)) {
+                log(field, String.format(ERROR_FIELD_ALONE, fieldName));
+            }
+        }
+    }
+
+    /*
+     * Get the field token from an assignation token.
+     * This method handles cases for fields referenced as `this.field` or only `field`
+     * It will get parameters from the method definition to ignore assignations to those parameters
+     */
+    private DetailAST getAssignedField(final DetailAST assignationToken) {
+        final Set<String> scopeParentParameterSet = getParameterSet(scopeParent.findFirstToken(
+            TokenTypes.PARAMETERS));
+        final DetailAST firstChild = assignationToken.getFirstChild();
+        final DetailAST assignationWithDot = firstChild.getType() == TokenTypes.DOT ? firstChild : null;
+
+        if (assignationWithDot != null) {
+            if (assignationWithDot.branchContains(TokenTypes.LITERAL_THIS)) {
+                return assignationWithDot.findFirstToken(TokenTypes.IDENT);
+            } else if (TokenUtil.findFirstTokenByPredicate(assignationWithDot,
+                token -> token.getText().equals(this.currentClassName)).isPresent()) {
+                // Case when referencing same class for private static fields
+                return assignationWithDot.getLastChild();
+            }
+        } else {
+            final DetailAST variableNameToken = assignationToken.getFirstChild();
+            // make sure the assignation is not for a method parameter
+            if (!scopeParentParameterSet.contains(variableNameToken.getText())) {
+                return variableNameToken;
+            }
+        }
+
+        return null;
+    }
+
+    /*
+     * Saves a field name to a container depending on the provided type
+     */
+    private void saveField(final String fieldName, final int scopeParentType) {
+        if (scopeParentType == TokenTypes.METHOD_DEF) {
+            assignmentsFromMethods.add(fieldName);
+        } else if (scopeParentType == TokenTypes.CTOR_DEF) {
+            assignmentsFromConstructor.add(fieldName);
+        }
+    }
+
+    /*
+     * Review an assignation to save fields that gets assigned in constructor or in any method
+     *
+     * @param assignationToken an assignation token
+     */
+    private void checkAssignation(final DetailAST assignationToken) {
+        if (scopeParent == null || assignationToken.getChildCount() == 0) {
+            // not inside any method or constructor definition. No need to check anything
+            // or this is an assignation from a notation like @Test(timeout = 5000) where assignation has not ChildCount
+            return;
+        }
+
+        final DetailAST assignationParent = assignationToken.getParent();
+        if (assignationParent != null && TokenTypes.VARIABLE_DEF == assignationParent.getType()) {
+            // Assignation for a variable definition. No need to check this assignation
+            return;
+        }
+
+        DetailAST fieldToken = getAssignedField(assignationToken);
+
+        if (fieldToken != null) {
+            saveField(fieldToken.getText(), scopeParent.getType());
+        }
+    }
+
+
+    /*
+     * Check each non-final field definition from a class and fill nonFinalFields
+     *
+     * @param classDefinitionAST a class definition AST
+     */
+    private void fillClassFieldDefinitions(DetailAST classDefinitionAST) {
+        final DetailAST classObjBlockAst = classDefinitionAST.findFirstToken(TokenTypes.OBJBLOCK);
+
+        TokenUtil.forEachChild(classObjBlockAst, TokenTypes.VARIABLE_DEF, (definitionToken) -> {
+            final DetailAST variableModifiersAst = definitionToken.findFirstToken(TokenTypes.MODIFIERS);
+            if (!variableModifiersAst.branchContains(TokenTypes.FINAL)
+                && !Utils.hasIllegalCombination(variableModifiersAst)) {
+                nonFinalFields.add(definitionToken);
+            }
+        });
+    }
+
+    /*
+     * Get a node AST with parameters definition and return the list of all parameter names
+     * The set of parameters is created the first time an assignation is check within a method or constructor
+     * and we don't need to generate it again until visiting a different method or constructor.
+     * Field `currentScopeParameterSet` ensures we don't create the set multiple times for the same method/constructor
+     *
+     * @param parametersAST a TokenTypes.PARAMETERS
+     * @return a set of parameter names
+     */
+    private Set<String> getParameterSet (DetailAST parametersAST) {
+        if (currentScopeParameterSet != null) {
+            return currentScopeParameterSet;
+        }
+        currentScopeParameterSet = new HashSet<>();
+        TokenUtil.forEachChild(parametersAST, TokenTypes.PARAMETER_DEF, (paramDefToken) -> {
+            final String parameterName = paramDefToken.findFirstToken(TokenTypes.IDENT).getText();
+            currentScopeParameterSet.add(parameterName);
+        });
+
+        return currentScopeParameterSet;
+    }
+}

eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/Utils.java

@@ -17,13 +17,23 @@
  * Common utils amount custom checks
  */
 public class Utils {
+    /*
+     * Set of modifiers that cannot be combined with final because it causes a violation.
+     */
     private static final Set INVALID_FINAL_COMBINATION = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(
         TokenTypes.LITERAL_TRANSIENT,
-        TokenTypes.LITERAL_VOLATILE
+        TokenTypes.LITERAL_VOLATILE,
+        TokenTypes.LITERAL_DEFAULT,
+        TokenTypes.LITERAL_PROTECTED
     )));
 
+    /*
+     * Set of annotations that cannot be combined with modifier 'final' because it would break serialization.
+     */
     private static final Set INVALID_FINAL_ANNOTATIONS = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(
-        "JsonProperty"
+        "JsonProperty",
+        "JsonAlias",
+        "JacksonXmlProperty"
     )));
 
     /**

eng/code-quality-reports/src/main/resources/checkstyle/checkstyle-suppressions.xml

@@ -101,16 +101,19 @@
   <suppress checks="com.azure.tools.checkstyle.checks.ServiceClientBuilderCheck" files=".*[/\\]src[/\\]test[/\\]java[/\\].*\.java"/>
   <suppress checks="com.azure.tools.checkstyle.checks.ServiceInterfaceCheck" files=".*[/\\]src[/\\]test[/\\]java[/\\].*\.java"/>
   <suppress checks="com.azure.tools.checkstyle.checks.HttpPipelinePolicyCheck" files=".*[/\\]src[/\\]test[/\\]java[/\\].*\.java"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.EnforceFinalFieldsCheck" files=".*[/\\]src[/\\]test[/\\]java[/\\].*\.java"/>
 
   <!-- Custom checkstyle rules that don't apply to files under implementation package -->
   <suppress checks="com.azure.tools.checkstyle.checks.ExternalDependencyExposedCheck" files=".*[/\\]implementation[/\\].*\.java"/>
   <suppress checks="com.azure.tools.checkstyle.checks.ServiceClientInstantiationCheck" files=".*[/\\]implementation[/\\].*\.java"/>
   <suppress checks="com.azure.tools.checkstyle.checks.ServiceClientBuilderCheck" files=".*[/\\]implementation[/\\].*\.java"/>
   <suppress checks="com.azure.tools.checkstyle.checks.ServiceInterfaceCheck" files=".*[/\\]implementation[/\\].*\.java"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.EnforceFinalFieldsCheck" files=".*[/\\]implementation[/\\].*\.java"/>
 
   <!-- Custom checkstyle rules that don't apply to files under samples package -->
   <suppress checks="com.azure.tools.checkstyle.checks.ExternalDependencyExposedCheck" files=".*[/\\]samples[/\\].*\.java"/>
   <suppress checks="com.azure.tools.checkstyle.checks.HttpPipelinePolicyCheck" files=".*[/\\]samples[/\\].*\.java"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.EnforceFinalFieldsCheck" files=".*[/\\]samples[/\\].*\.java"/>
 
   <!-- Don't apply custom Checkstyle rules to files under checkstyle package. -->
   <suppress checks="com\.azure\.tools\.checkstyle\.checks\..+" files=".*[/\\]tools[/\\]checkstyle[/\\].*"/>

eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml

@@ -317,6 +317,10 @@ page at http://checkstyle.sourceforge.net/config.html -->
     1) All fields must be final -->
     <module name="com.azure.tools.checkstyle.checks.OnlyFinalFieldsForImmutableClassCheck"/>
 
+    <!-- CUSTOM CHECKS -->
+    <!-- Verify the whenever a field is assigned just once in constructor to be final -->
+    <module name="com.azure.tools.checkstyle.checks.EnforceFinalFieldsCheck"/>
+
     <!--CUSTOM CHECKS-->
     <!-- Must use 'logger.logAndThrow' but not directly calling 'throw exception' -->
 <!--    <module name="com.azure.tools.checkstyle.checks.ThrownClientLoggerCheck"/>-->

sdk/appconfiguration/azure-data-appconfiguration/src/main/java/com/azure/data/appconfiguration/ConfigurationClientBuilder.java

@@ -83,7 +83,7 @@ public final class ConfigurationClientBuilder {
     private HttpClient httpClient;
     private HttpLogDetailLevel httpLogDetailLevel;
     private HttpPipeline pipeline;
-    private RetryPolicy retryPolicy;
+    private final RetryPolicy retryPolicy;
     private Configuration configuration;
 
     /**

sdk/appconfiguration/azure-data-appconfiguration/src/main/java/com/azure/data/appconfiguration/credentials/ConfigurationClientCredentials.java

@@ -149,9 +149,9 @@ private static class CredentialInformation {
         private static final String ID = "id=";
         private static final String SECRET = "secret=";
 
-        private URL baseUri;
-        private String id;
-        private byte[] secret;
+        private final URL baseUri;
+        private final String id;
+        private final byte[] secret;
 
         URL baseUri() {
             return baseUri;
@@ -175,24 +175,32 @@ byte[] secret() {
                 throw new IllegalArgumentException("invalid connection string segment count");
             }
 
+            URL baseUri = null;
+            String id = null;
+            byte[] secret = null;
+
             for (String arg : args) {
                 String segment = arg.trim();
                 String lowerCase = segment.toLowerCase(Locale.US);
 
                 if (lowerCase.startsWith(ENDPOINT)) {
                     try {
-                        this.baseUri = new URL(segment.substring(ENDPOINT.length()));
+                        baseUri = new URL(segment.substring(ENDPOINT.length()));
                     } catch (MalformedURLException ex) {
                         throw new IllegalArgumentException(ex);
                     }
                 } else if (lowerCase.startsWith(ID)) {
-                    this.id = segment.substring(ID.length());
+                    id = segment.substring(ID.length());
                 } else if (lowerCase.startsWith(SECRET)) {
                     String secretBase64 = segment.substring(SECRET.length());
-                    this.secret = Base64.getDecoder().decode(secretBase64);
+                    secret = Base64.getDecoder().decode(secretBase64);
                 }
             }
 
+            this.baseUri = baseUri;
+            this.id = id;
+            this.secret = secret;
+
             if (this.baseUri == null || this.id == null || this.secret == null) {
                 throw new IllegalArgumentException("Could not parse 'connectionString'."
                     + " Expected format: 'endpoint={endpoint};id={id};secret={secret}'. Actual:" + connectionString);

sdk/core/azure-core-management/src/main/java/com/azure/core/management/CloudError.java

@@ -28,7 +28,7 @@ public final class CloudError {
     /**
      * Details for the error.
      */
-    private List<CloudError> details;
+    private final List<CloudError> details;
 
     /**
      * Initializes a new instance of CloudError.

sdk/core/azure-core-management/src/main/java/com/azure/core/management/PagedList.java

@@ -22,7 +22,7 @@
  */
 public abstract class PagedList<E> implements List<E> {
     /** The actual items in the list. */
-    private List<E> items;
+    private final List<E> items;
     /** Stores the latest page fetched. */
     private Page<E> currentPage;
     /** Cached page right after the current one. */

sdk/core/azure-core-management/src/main/java/com/azure/core/management/serializer/CloudErrorDeserializer.java

@@ -19,7 +19,7 @@
  */
 final class CloudErrorDeserializer extends JsonDeserializer<CloudError> {
     /** Object mapper for default deserializations. */
-    private ObjectMapper mapper;
+    private final ObjectMapper mapper;
 
     /**
      * Creates an instance of CloudErrorDeserializer.