[Parser] Cleans up parsing of parameter attributes. Implements SE-0053. Fixes SR-979, SR-1020 and cleans up implementation of SE-0003. Provides better fix-its and diagnostics for misplaced 'inout' and prohibits 'var' and 'let' from parameter attributes

Author: manavgabhawala

include/swift/AST/DiagnosticsParse.def

@@ -148,9 +148,6 @@ WARNING(lex_editor_placeholder_in_playground,none,
 
 NOTE(note_in_decl_extension,none,
      "in %select{declaration|extension}0 of %1", (bool, Identifier))
-WARNING(inout_as_attr_deprecated,none,
-        "'inout' before a parameter name is deprecated, place it before the parameter type instead",
-        ())
 WARNING(line_directive_style_deprecated,none,
         "#line directive is deprecated, please use #sourceLocation instead",
         ())
@@ -692,12 +689,15 @@ ERROR(multiple_parameter_ellipsis,none,
       "only a single variadic parameter '...' is permitted", ())
 ERROR(parameter_vararg_default,none,
       "variadic parameter cannot have a default value", ())
-ERROR(parameter_inout_var_let,none,
+ERROR(inout_as_attr_disallowed,none,
+      "'inout' before a parameter name is not allowed, place it before the parameter type instead",
+      ())
+ERROR(parameter_inout_var_let_repeated,none,
       "parameter may not have multiple 'inout', 'var', or 'let' specifiers",
       ())
+ERROR(parameter_let_as_attr,none,
+      "'let' as a parameter attribute is not allowed", ())
 
-ERROR(var_parameter_not_allowed,none,
-        "parameters may not have the 'var' specifier", ())
 
 ERROR(expected_behavior_name,none,
       "expected behavior name after '[' in property declaration", ())

include/swift/AST/DiagnosticsSema.def

@@ -778,6 +778,9 @@ ERROR(inout_cant_be_variadic,none,
       "inout arguments cannot be variadic", ())
 ERROR(inout_only_parameter,none,
       "'inout' is only valid in parameter lists", ())
+ERROR(var_parameter_not_allowed,none,
+      "parameters may not have the 'var' specifier", ())
+
 
 ERROR(mutating_invalid_global_scope,none,
       "'mutating' is only valid on methods", ())

include/swift/Parse/Parser.h

@@ -933,7 +933,6 @@ class Parser {
     DeclAttributes Attrs;
 
     /// The location of the 'let', 'var', or 'inout' keyword, if present.
-    ///
     SourceLoc LetVarInOutLoc;
 
     enum SpecifierKindTy {

lib/Parse/ParsePattern.cpp

@@ -186,27 +186,31 @@ Parser::parseParameterClause(SourceLoc &leftParenLoc,
         status |= makeParserCodeCompletionStatus();
       }
     }
+    
     // ('inout' | 'let' | 'var')?
-    if (Tok.is(tok::kw_inout)) {
-      param.LetVarInOutLoc = consumeToken();
-      param.SpecifierKind = ParsedParameter::InOut;
-    } else if (Tok.is(tok::kw_let)) {
-      param.LetVarInOutLoc = consumeToken();
-      param.SpecifierKind = ParsedParameter::Let;
-    } else if (Tok.is(tok::kw_var)) {
-      diagnose(Tok.getLoc(), diag::var_parameter_not_allowed)
-        .fixItRemove(Tok.getLoc());
-      param.LetVarInOutLoc = consumeToken();
-      param.SpecifierKind = ParsedParameter::Var;
-    }
-
-    // Redundant specifiers are fairly common, recognize, reject, and recover
-    // from this gracefully.
-    if (Tok.isAny(tok::kw_inout, tok::kw_let, tok::kw_var)) {
-      diagnose(Tok, diag::parameter_inout_var_let)
+    bool hasSpecifier = false;
+    while (Tok.isAny(tok::kw_inout, tok::kw_let, tok::kw_var)) {
+      if (!hasSpecifier) {
+        if (Tok.is(tok::kw_let)) {
+          diagnose(Tok, diag::parameter_let_as_attr)
+          .fixItRemove(Tok.getLoc());
+          param.isInvalid = true;
+        } else {
+          // We handle the var error in sema for a better fixit and inout is
+          // handled later in this function for better fixits.
+          param.SpecifierKind = Tok.is(tok::kw_inout) ? ParsedParameter::InOut :
+                                                        ParsedParameter::Var;
+        }
+        param.LetVarInOutLoc = consumeToken();
+        hasSpecifier = true;
+      } else {
+        // Redundant specifiers are fairly common, recognize, reject, and recover
+        // from this gracefully.
+        diagnose(Tok, diag::parameter_inout_var_let_repeated)
         .fixItRemove(Tok.getLoc());
-      consumeToken();
-      param.isInvalid = true;
+        consumeToken();
+        param.isInvalid = true;
+      }
     }
 
     if (startsParameterName(*this, isClosure)) {
@@ -248,20 +252,28 @@ Parser::parseParameterClause(SourceLoc &leftParenLoc,
 
         bool hasDeprecatedInOut =
           param.SpecifierKind == ParsedParameter::InOut;
-
-        if (Tok.is(tok::kw_inout)) {
-          param.LetVarInOutLoc = consumeToken();
-          param.SpecifierKind = ParsedParameter::InOut;
-          if (hasDeprecatedInOut) {
-            diagnose(param.LetVarInOutLoc, diag::inout_as_attr_deprecated)
-              .fixItRemove(param.LetVarInOutLoc);
+        bool hasValidInOut = false;
+        
+        while (Tok.is(tok::kw_inout)) {
+          hasValidInOut = true;
+          if (hasSpecifier) {
+            diagnose(Tok.getLoc(), diag::parameter_inout_var_let_repeated)
+            .fixItRemove(param.LetVarInOutLoc);
+            consumeToken(tok::kw_inout);
+            param.isInvalid = true;
+          } else {
+            hasSpecifier = true;
+            param.LetVarInOutLoc = consumeToken(tok::kw_inout);
+            param.SpecifierKind = ParsedParameter::InOut;
           }
-        } else if (hasDeprecatedInOut) {
-          diagnose(param.LetVarInOutLoc, diag::inout_as_attr_deprecated)
-            .fixItRemove(param.LetVarInOutLoc)
-            .fixItInsert(postColonLoc, "inout ");
         }
-
+        if (!hasValidInOut && hasDeprecatedInOut) {
+          diagnose(Tok.getLoc(), diag::inout_as_attr_disallowed)
+          .fixItRemove(param.LetVarInOutLoc)
+          .fixItInsert(postColonLoc, "inout ");
+          param.isInvalid = true;
+        }
+        
         auto type = parseType(diag::expected_parameter_type);
         status |= type;
         param.Type = type.getPtrOrNull();

lib/Sema/TypeCheckPattern.cpp

@@ -711,6 +711,73 @@ static bool validateTypedPattern(TypeChecker &TC, DeclContext *DC,
   return hadError;
 }
 
+static void diagnoseAndMigrateVarParameterToBody(ParamDecl *decl,
+                                                 AbstractFunctionDecl *func,
+                                                 TypeChecker &TC) {
+  if (!func || !func->hasBody()) {
+    // If there is no function body, just suggest removal.
+    TC.diagnose(decl->getLetVarInOutLoc(),
+                diag::var_parameter_not_allowed)
+      .fixItRemove(decl->getLetVarInOutLoc());
+    return;
+  }
+  // Insert the shadow copy. The computations that follow attempt to
+  // 'best guess' the indentation and new lines so that the user
+  // doesn't have to add any whitespace.
+  auto declBody = func->getBody();
+  
+  auto &SM = TC.Context.SourceMgr;
+  
+  SourceLoc insertionStartLoc;
+  std::string start;
+  std::string end;
+  
+  auto lBraceLine = SM.getLineNumber(declBody->getLBraceLoc());
+  auto rBraceLine = SM.getLineNumber(declBody->getRBraceLoc());
+
+  if (!declBody->getNumElements()) {
+    
+    // Empty function body.
+    insertionStartLoc = declBody->getRBraceLoc();
+    
+    if (lBraceLine == rBraceLine) {
+      // Same line braces, means we probably have something
+      // like {} as the func body. Insert directly into body with spaces.
+      start = " ";
+      end = " ";
+    } else {
+      // Different line braces, so use RBrace's indentation.
+      end = "\n" + Lexer::getIndentationForLine(SM, declBody->
+                                                getRBraceLoc()).str();
+      start = "    "; // Guess 4 spaces as extra indentation.
+    }
+  } else {
+    auto firstLine = declBody->getElement(0);
+    insertionStartLoc = firstLine.getStartLoc();
+    if (lBraceLine == SM.getLineNumber(firstLine.getStartLoc())) {
+      // Function on same line, insert with semi-colon. Not ideal but
+      // better than weird space alignment.
+      start = "";
+      end = "; ";
+    } else {
+      start = "";
+      end = "\n" + Lexer::getIndentationForLine(SM, firstLine.
+                                                getStartLoc()).str();
+    }
+  }
+  if (insertionStartLoc.isInvalid()) {
+    TC.diagnose(decl->getLetVarInOutLoc(),
+                diag::var_parameter_not_allowed)
+    .fixItRemove(decl->getLetVarInOutLoc());
+    return;
+  }
+  auto parameterName = decl->getNameStr().str();
+  TC.diagnose(decl->getLetVarInOutLoc(),
+              diag::var_parameter_not_allowed)
+  .fixItRemove(decl->getLetVarInOutLoc())
+  .fixItInsert(insertionStartLoc, start + "var " + parameterName + " = " +
+               parameterName + end);
+}
 
 static bool validateParameterType(ParamDecl *decl, DeclContext *DC,
                                   TypeResolutionOptions options,
@@ -736,6 +803,15 @@ static bool validateParameterType(ParamDecl *decl, DeclContext *DC,
     }
     decl->getTypeLoc().setType(Ty);
   }
+  // If the param is not a 'let' and it is not an 'inout'.
+  // It must be a 'var'. Provide helpful diagnostics like a shadow copy
+  // in the function body to fix the 'var' attribute.
+  if (!decl->isLet() && !Ty->is<InOutType>()) {
+    auto func = dyn_cast_or_null<AbstractFunctionDecl>(DC);
+    diagnoseAndMigrateVarParameterToBody(decl, func, TC);
+    decl->setInvalid();
+    hadError = true;
+  }
 
   if (hadError)
     decl->getTypeLoc().setType(ErrorType::get(TC.Context), /*validated*/true);

test/Constraints/tuple.swift

@@ -103,7 +103,7 @@ func testLValue(c: C) {
 
 
 // <rdar://problem/21444509> Crash in TypeChecker::coercePatternToType
-func invalidPatternCrash(let k : Int) {
+func invalidPatternCrash(k : Int) {
   switch k {
   case (k, cph_: k) as UInt8:  // expected-error {{tuple pattern cannot match values of the non-tuple type 'UInt8'}} expected-warning {{cast from 'Int' to unrelated type 'UInt8' always fails}}
     break

test/FixCode/fixits-apply-all.swift

@@ -7,3 +7,13 @@
 func ftest1() {
   let myvar = 0
 }
+
+func foo() -> Int {
+  do {
+  } catch var err {
+    goo(err)
+  }
+}
+func goo(e: ErrorProtocol) {
+	
+}

test/FixCode/fixits-apply-all.swift.result

@@ -7,3 +7,13 @@
 func ftest1() {
   _ = 0
 }
+
+func foo() -> Int {
+  do {
+  } catch let err {
+    goo(err)
+  }
+}
+func goo(e: ErrorProtocol) {
+	
+}

test/FixCode/fixits-apply.swift

@@ -31,14 +31,25 @@ func supported() -> MyMask {
   return Int(MyMask.Bingo.rawValue)
 }
 
-func foo() -> Int {
-  do {
-  } catch var err {
-    goo(err)
+func goo(var e : ErrorProtocol) {
+}
+func goo2(var e: ErrorProtocol) {}
+func goo3(var e: Int) { e = 3 }
+protocol A {
+  func bar(var s: Int)
+}
+extension A {
+  func bar(var s: Int) {
+    s += 5
   }
 }
 
-func goo(var e : ErrorProtocol) {}
+func baz(var x: Int) {
+  x += 10
+}
+func foo(let y: String, inout x: Int) {
+  
+}
 
 struct Test1 : OptionSet {
   init(rawValue: Int) {}

test/FixCode/fixits-apply.swift.result

@@ -31,14 +31,28 @@ func supported() -> MyMask {
   return MyMask.Bingo
 }
 
-func foo() -> Int {
-  do {
-  } catch let err {
-    goo(err)
+func goo(e : ErrorProtocol) {
+    var e = e
+}
+func goo2(e: ErrorProtocol) { var e = e }
+func goo3(e: Int) { var e = e; e = 3 }
+protocol A {
+  func bar(s: Int)
+}
+extension A {
+  func bar(s: Int) {
+    var s = s
+    s += 5
   }
 }
 
-func goo(e : ErrorProtocol) {}
+func baz(x: Int) {
+  var x = x
+  x += 10
+}
+func foo(y: String, x: inout Int) {
+  
+}
 
 struct Test1 : OptionSet {
   init(rawValue: Int) {}

test/IDE/complete_from_stdlib.swift

@@ -229,7 +229,7 @@ func testPostfixOperator1(x: Int) {
 // POSTFIX_RVALUE_INT-NOT: ++
 // POSTFIX_RVALUE_INT-NOT: --
 
-func testPostfixOperator2(var x: Int) {
+func testPostfixOperator2(x: inout Int) {
   x#^POSTFIX_INT_2^#
 }
 // POSTFIX_LVALUE_INT: Decl[PostfixOperatorFunction]/OtherModule[Swift]: ++[#Int#]; name=
@@ -253,7 +253,7 @@ func testInfixOperator1(x: Int) {
 // INFIX_INT: End completions
 // NEGATIVE_INFIX_INT-NOT: &&
 // NEGATIVE_INFIX_INT-NOT: +=
-func testInfixOperator2(var x: Int) {
+func testInfixOperator2(x: inout Int) {
   x#^INFIX_INT_2^#
 }
 // INFIX_LVALUE_INT: Begin completions

test/IDE/complete_operators.swift

@@ -66,7 +66,7 @@ func testPostfix1(x: S) {
 }
 // POSTFIX_1-NOT: ++
 
-func testPostfix2(var x: S) {
+func testPostfix2(x: inout S) {
   x#^POSTFIX_2^#
 }
 // POSTFIX_2: Begin completions
@@ -130,7 +130,7 @@ func testPostfix10<G: P where G.T : Fooable>(x: G) {
 }
 // POSTFIX_10: Decl[PostfixOperatorFunction]/CurrModule: ***[#G.T#]
 
-func testPostfixSpace(var x: S) {
+func testPostfixSpace(x: inout S) {
   x #^S_POSTFIX_SPACE^#
 }
 // S_POSTFIX_SPACE: Decl[PostfixOperatorFunction]/CurrModule/Erase[1]:  ++[#S#]
@@ -167,7 +167,7 @@ func testInfix1(x: S2) {
 // NEGATIVE_S2_INFIX-NOT: ~>
 // NEGATIVE_S2_INFIX-NOT: = {#
 
-func testInfix2(var x: S2) {
+func testInfix2(x: inout S2) {
   x#^INFIX_2^#
 }
 // S2_INFIX_LVALUE: Begin completions
@@ -296,7 +296,7 @@ func testSpace(x: S2) {
 // S2_INFIX_SPACE-DAG: Decl[InfixOperatorFunction]/OtherModule[Swift]: [' ']+ {#S2#}[#S2#]
 // S2_INFIX_SPACE: End completions
 
-func testExtInfix1(var x: S2) {
+func testExtInfix1(x: inout S2) {
   x + S2() + x + S2() + x + S2() + x#^EXT_INFIX_1^#
 }