[private named parameters] Update compile errors

Change how the parser reports errors for named parameters that start with `_` in anticipation of supporting private named parameters.

With "private named parameters" some private named parameters are errors and some aren't. It's still an error to have a private named parameter that doesn't refer to a field. This updates the error reporting to implement that. It also consolidates the error reporting into the parser. Prior to this CL, analyzer would discard the parser's private named parameter errors and then re-detect and report them itself.

The errors you get now when the experiment is off are:

* If the parameter doesn't refer to a field, you get the old error that you can't have a private named parameter.

* If the parameter does refer to a field, you get an error telling you to enable the experiment. When the experiment ships, this changes to an error about updating the library's language version.

And when the experiment is on:

* If the parameter doesn't refer to a field, you get a new error that you can't have a private named parameter unless it refers to a field.

* If the parameter does refer to a field, it reports no error. It doesn't actually support private named parameters that refer to fields yet.

Let me know what I got wrong. It was a little tricky to figure out how it should behave when the experiment is off. One option is to have no user-visible changes in behavior at all when the experiment is off. So the feature is fully hidden behind the experiment.

But most other code I saw seems to take an approach where if the feature is off, it acknowledges the existence of the feature but tells the user it's disabled. I went with this approach but I don't know if that's something we should only do when it's closer to shipping.

Change-Id: I0fc4e9b06c8e7f0abc868e0f49d2b3e625c8dade
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/456540
Auto-Submit: Bob Nystrom <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
Reviewed-by: Phil Quitslund <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Phil Quitslund <[email protected]>

Author: munificent

pkg/_fe_analyzer_shared/lib/src/messages/codes_generated.dart

@@ -2684,6 +2684,14 @@ const MessageCode codePrefixAfterCombinator = const MessageCode(
   correctionMessage: """Try moving the prefix before the combinators.""",
 );
 
+// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
+const MessageCode codePrivateNamedNonFieldParameter = const MessageCode(
+  "PrivateNamedNonFieldParameter",
+  pseudoSharedCode: PseudoSharedCode.privateNamedNonFieldParameter,
+  problemMessage:
+      """A named parameter that doesn't refer to an instance variable can't start with an underscore ('_').""",
+);
+
 // DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
 const MessageCode codePrivateNamedParameter = const MessageCode(
   "PrivateNamedParameter",
@@ -3203,6 +3211,7 @@ enum PseudoSharedCode {
   nonSyncAbstractMethod,
   nonSyncFactory,
   positionalAfterNamedArgument,
+  privateNamedNonFieldParameter,
   privateOptionalParameter,
   returnInGenerator,
   setOrMapLiteralTooManyTypeArguments,

pkg/_fe_analyzer_shared/lib/src/parser/parser_impl.dart

@@ -328,6 +328,9 @@ class Parser {
   /// `true` if the 'declaring-constructors' feature is enabled.
   final bool _isDeclaringConstructorsFeatureEnabled;
 
+  /// `true` if the 'private-named-parameters' feature is enabled.
+  final bool _isPrivateNamedParametersEnabled;
+
   /// Indicates whether the last pattern parsed is allowed inside unary
   /// patterns.  This is set by [parsePrimaryPattern] and [parsePattern].
   ///
@@ -352,7 +355,9 @@ class Parser {
        _isEnhancedPartsFeatureEnabled = experimentalFeatures
            .isExperimentEnabled(ExperimentalFlag.enhancedParts),
        _isDeclaringConstructorsFeatureEnabled = experimentalFeatures
-           .isExperimentEnabled(ExperimentalFlag.declaringConstructors);
+           .isExperimentEnabled(ExperimentalFlag.declaringConstructors),
+       _isPrivateNamedParametersEnabled = experimentalFeatures
+           .isExperimentEnabled(ExperimentalFlag.privateNamedParameters);
 
   /// Executes [callback]; however if `this` is the `TestParser` (from
   /// `pkg/front_end/test/parser_test_parser.dart`) then no output is printed
@@ -2300,7 +2305,29 @@ class Parser {
     } else {
       nameToken = token = ensureIdentifier(token, nameContext);
       if (isNamedParameter && nameToken.lexeme.startsWith("_")) {
-        reportRecoverableError(nameToken, codes.codePrivateNamedParameter);
+        // TODO(rnystrom): Also handle declaring field parameters.
+        bool refersToField = thisKeyword != null;
+
+        if (_isPrivateNamedParametersEnabled) {
+          if (!refersToField) {
+            reportRecoverableError(
+              nameToken,
+              codes.codePrivateNamedNonFieldParameter,
+            );
+          }
+        } else {
+          if (!refersToField) {
+            reportRecoverableError(nameToken, codes.codePrivateNamedParameter);
+          } else {
+            // The user is using syntax that is now meaningful, but in a
+            // library where it isn't enabled, so report a more precise error.
+            reportExperimentNotEnabled(
+              ExperimentalFlag.privateNamedParameters,
+              nameToken,
+              nameToken,
+            );
+          }
+        }
       }
     }
     if (endInlineFunctionType != null) {

pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml

@@ -1378,10 +1378,6 @@ CompileTimeErrorCode.PREFIX_SHADOWED_BY_LOCAL_DECLARATION:
   status: noFix
 CompileTimeErrorCode.PRIVATE_COLLISION_IN_MIXIN_APPLICATION:
   status: noFix
-CompileTimeErrorCode.PRIVATE_OPTIONAL_PARAMETER:
-  status: needsFix
-  notes: |-
-    Remove the underscore
 CompileTimeErrorCode.PRIVATE_SETTER:
   status: noFix
 CompileTimeErrorCode.READ_POTENTIALLY_UNASSIGNED_FINAL:
@@ -3221,6 +3217,12 @@ ParserErrorCode.PREFIX_AFTER_COMBINATOR:
   status: needsFix
   notes: |-
     Move the prefix before the combinators.
+ParserErrorCode.PRIVATE_OPTIONAL_PARAMETER:
+  status: needsFix
+  notes: |-
+    Remove the underscore
+ParserErrorCode.PRIVATE_NAMED_NON_FIELD_PARAMETER:
+  status: needsEvaluation
 ParserErrorCode.RECORD_LITERAL_ONE_POSITIONAL_NO_TRAILING_COMMA:
   status: hasFix
 ParserErrorCode.RECORD_TYPE_ONE_POSITIONAL_NO_TRAILING_COMMA:

pkg/analysis_server/test/src/services/correction/fix/remove_ignored_diagnostic_test.dart

@@ -165,12 +165,12 @@ class C {
   Future<void> test_line_first_leading_and_eol() async {
     await resolveTestCode('''
 class C {
-  // ignore: private_optional_parameter
+  // ignore: private_named_non_field_parameter
   int f({required _a}) => null; // ignore: unused_local_variable, return_of_invalid_type
 }''');
     await assertHasFix('''
 class C {
-  // ignore: private_optional_parameter
+  // ignore: private_named_non_field_parameter
   int f({required _a}) => null; // ignore: return_of_invalid_type
 }''');
   }

pkg/analyzer/lib/src/dart/error/syntactic_errors.g.dart

@@ -2298,6 +2298,24 @@ class ParserErrorCode extends DiagnosticCodeWithExpectedTypes {
     expectedTypes: [],
   );
 
+  /// No parameters.
+  static const ParserErrorWithoutArguments
+  privateNamedNonFieldParameter = ParserErrorWithoutArguments(
+    'PRIVATE_NAMED_NON_FIELD_PARAMETER',
+    "Named parameters that don't refer to instance variables can't start with "
+        "underscore.",
+    expectedTypes: [],
+  );
+
+  /// No parameters.
+  static const ParserErrorWithoutArguments privateOptionalParameter =
+      ParserErrorWithoutArguments(
+        'PRIVATE_OPTIONAL_PARAMETER',
+        "Named parameters can't start with an underscore.",
+        hasPublishedDocs: true,
+        expectedTypes: [],
+      );
+
   /// No parameters.
   static const ParserErrorWithoutArguments
   recordLiteralOnePositionalNoTrailingComma = ParserErrorWithoutArguments(

pkg/analyzer/lib/src/diagnostic/diagnostic_code_values.g.dart

@@ -463,7 +463,6 @@ const List<DiagnosticCode> diagnosticCodeValues = [
   CompileTimeErrorCode.prefixIdentifierNotFollowedByDot,
   CompileTimeErrorCode.prefixShadowedByLocalDeclaration,
   CompileTimeErrorCode.privateCollisionInMixinApplication,
-  CompileTimeErrorCode.privateOptionalParameter,
   CompileTimeErrorCode.privateSetter,
   CompileTimeErrorCode.readPotentiallyUnassignedFinal,
   CompileTimeErrorCode.recordLiteralOnePositionalNoTrailingCommaByType,
@@ -875,6 +874,8 @@ const List<DiagnosticCode> diagnosticCodeValues = [
   ParserErrorCode.positionalAfterNamedArgument,
   ParserErrorCode.positionalParameterOutsideGroup,
   ParserErrorCode.prefixAfterCombinator,
+  ParserErrorCode.privateNamedNonFieldParameter,
+  ParserErrorCode.privateOptionalParameter,
   ParserErrorCode.recordLiteralOnePositionalNoTrailingComma,
   ParserErrorCode.recordTypeOnePositionalNoTrailingComma,
   ParserErrorCode.redirectingConstructorWithBody,

pkg/analyzer/lib/src/error/codes.g.dart

@@ -6065,15 +6065,6 @@ class CompileTimeErrorCode extends DiagnosticCodeWithExpectedTypes {
     ],
   );
 
-  /// No parameters.
-  static const CompileTimeErrorWithoutArguments privateOptionalParameter =
-      CompileTimeErrorWithoutArguments(
-        'PRIVATE_OPTIONAL_PARAMETER',
-        "Named parameters can't start with an underscore.",
-        hasPublishedDocs: true,
-        expectedTypes: [],
-      );
-
   /// Parameters:
   /// String p0: the name of the setter
   static const CompileTimeErrorTemplate<

pkg/analyzer/lib/src/fasta/error_converter.dart

@@ -333,7 +333,18 @@ class FastaErrorReporter {
         // Reported by [ErrorVerifier._checkForBuiltInIdentifierAsName].
         return;
       case PseudoSharedCode.privateOptionalParameter:
-        // Reported by [ErrorVerifier._checkForPrivateOptionalParameter].
+        diagnosticReporter?.atOffset(
+          offset: offset,
+          length: length,
+          diagnosticCode: ParserErrorCode.privateOptionalParameter,
+        );
+        return;
+      case PseudoSharedCode.privateNamedNonFieldParameter:
+        diagnosticReporter?.atOffset(
+          offset: offset,
+          length: length,
+          diagnosticCode: ParserErrorCode.privateNamedNonFieldParameter,
+        );
         return;
       case PseudoSharedCode.nonSyncAbstractMethod:
         // Not reported but followed by a MISSING_FUNCTION_BODY error.

pkg/analyzer/lib/src/generated/error_verifier.dart

@@ -930,7 +930,6 @@ class ErrorVerifier extends RecursiveAstVisitor<void>
   @override
   void visitFieldFormalParameter(FieldFormalParameter node) {
     _checkForValidField(node);
-    _checkForPrivateOptionalParameter(node);
     _checkForFieldInitializingFormalRedirectingConstructor(node);
     _checkForTypeAnnotationDeferredClass(node.type);
     var fieldElement = node.declaredFragment?.element.field;
@@ -1456,7 +1455,6 @@ class ErrorVerifier extends RecursiveAstVisitor<void>
 
   @override
   void visitSimpleFormalParameter(SimpleFormalParameter node) {
-    _checkForPrivateOptionalParameter(node);
     _checkForTypeAnnotationDeferredClass(node.type);
     super.visitSimpleFormalParameter(node);
   }
@@ -4958,24 +4956,6 @@ class ErrorVerifier extends RecursiveAstVisitor<void>
     }
   }
 
-  /// Check that the given named optional [parameter] does not begin with '_'.
-  void _checkForPrivateOptionalParameter(FormalParameter parameter) {
-    // should be named parameter
-    if (!parameter.isNamed) {
-      return;
-    }
-    // name should start with '_'
-    var name = parameter.name;
-    if (name == null || name.isSynthetic || !name.lexeme.startsWith('_')) {
-      return;
-    }
-
-    diagnosticReporter.atToken(
-      name,
-      CompileTimeErrorCode.privateOptionalParameter,
-    );
-  }
-
   /// Check whether the given constructor [declaration] is the redirecting
   /// generative constructor and references itself directly or indirectly. The
   /// [constructorElement] is the constructor element.

pkg/analyzer/messages.yaml

@@ -13901,36 +13901,6 @@ CompileTimeErrorCode:
 
       If you need both of the mixins, then rename the conflicting member in one
       of the two mixins.
-  PRIVATE_OPTIONAL_PARAMETER:
-    parameters: none
-    problemMessage: "Named parameters can't start with an underscore."
-    hasPublishedDocs: true
-    documentation: |-
-      #### Description
-
-      The analyzer produces this diagnostic when the name of a named parameter
-      starts with an underscore.
-
-      #### Example
-
-      The following code produces this diagnostic because the named parameter
-      `_x` starts with an underscore:
-
-      ```dart
-      class C {
-        void m({int [!_x!] = 0}) {}
-      }
-      ```
-
-      #### Common fixes
-
-      Rename the parameter so that it doesn't start with an underscore:
-
-      ```dart
-      class C {
-        void m({int x = 0}) {}
-      }
-      ```
   PRIVATE_SETTER:
     parameters:
       String p0: the name of the setter
@@ -21788,6 +21758,69 @@ ParserErrorCode:
     problemMessage: "Positional parameters must be enclosed in square brackets ('[' and ']')."
     correctionMessage: Try surrounding the positional parameters in square brackets.
     hasPublishedDocs: false
+  PRIVATE_OPTIONAL_PARAMETER:
+    parameters: none
+    problemMessage: "Named parameters can't start with an underscore."
+    hasPublishedDocs: true
+    documentation: |-
+      #### Description
+
+      The analyzer produces this diagnostic when the name of a named parameter
+      starts with an underscore.
+
+      #### Example
+
+      The following code produces this diagnostic because the named parameter
+      `_x` starts with an underscore:
+
+      ```dart
+      class C {
+        void m({int [!_x!] = 0}) {}
+      }
+      ```
+
+      #### Common fixes
+
+      Rename the parameter so that it doesn't start with an underscore:
+
+      ```dart
+      class C {
+        void m({int x = 0}) {}
+      }
+      ```
+  PRIVATE_NAMED_NON_FIELD_PARAMETER:
+    parameters: none
+    problemMessage: "Named parameters that don't refer to instance variables can't start with underscore."
+    hasPublishedDocs: false
+    documentation: |-
+      #### Description
+
+      The analyzer produces this diagnostic when the name of a named parameter
+      starts with an underscore and the parameter isn't an initializing formal
+      or field parameter.
+
+      #### Example
+
+      The following code produces this diagnostic because the named parameter
+      `_x` starts with an underscore:
+
+      ```dart
+      class C {
+        C({int [!_x!] = 0});
+      }
+      ```
+
+      #### Common fixes
+
+      Only use a private named parameter when it refers to a field:
+
+      ```dart
+      class C {
+        final int _x;
+        C({this._x = 0});
+        int get x => _x;
+      }
+      ```
   REPRESENTATION_FIELD_MODIFIER:
     parameters: none
     problemMessage: "Representation fields can't have modifiers."