From 06043cd6615e6b48f62fa5f349c935553cf50c35 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 28 Apr 2019 12:27:37 +0200 Subject: [PATCH 01/12] Syntax change: streamline ascriptions in match scrutinees Allow ascriptions in match scrutinees without requiring parentheses. --- .../src/dotty/tools/dotc/parsing/Parsers.scala | 18 ++++++++++-------- docs/docs/internals/syntax.md | 7 ++++--- .../fatal-warnings/unchecked-scrutinee.scala | 5 +++++ 3 files changed, 19 insertions(+), 11 deletions(-) create mode 100644 tests/pos-special/fatal-warnings/unchecked-scrutinee.scala diff --git a/compiler/src/dotty/tools/dotc/parsing/Parsers.scala b/compiler/src/dotty/tools/dotc/parsing/Parsers.scala index aa63467ad1b0..3751b3f869d0 100644 --- a/compiler/src/dotty/tools/dotc/parsing/Parsers.scala +++ b/compiler/src/dotty/tools/dotc/parsing/Parsers.scala @@ -1200,14 +1200,15 @@ object Parsers { * | ForExpr * | [SimpleExpr `.'] id `=' Expr * | SimpleExpr1 ArgumentExprs `=' Expr - * | PostfixExpr [Ascription] - * | [‘inline’] PostfixExpr `match' `{' CaseClauses `}' + * | Expr2 + * | [‘inline’] Expr2 `match' `{' CaseClauses `}' * | `implicit' `match' `{' ImplicitCaseClauses `}' - * Bindings ::= `(' [Binding {`,' Binding}] `)' - * Binding ::= (id | `_') [`:' Type] - * Ascription ::= `:' CompoundType - * | `:' Annotation {Annotation} - * | `:' `_' `*' + * Bindings ::= `(' [Binding {`,' Binding}] `)' + * Binding ::= (id | `_') [`:' Type] + * Expr2 ::= PostfixExpr [Ascription] + * Ascription ::= `:' InfixType + * | `:' Annotation {Annotation} + * | `:' `_' `*' */ val exprInParens: () => Tree = () => expr(Location.InParens) @@ -1324,7 +1325,8 @@ object Parsers { t } case COLON => - ascription(t, location) + val t1 = ascription(t, location) + if (in.token == MATCH) expr1Rest(t1, location) else t1 case MATCH => matchExpr(t, startOffset(t), Match) case _ => diff --git a/docs/docs/internals/syntax.md b/docs/docs/internals/syntax.md index 97a1fd161eb5..17a0aebceeb4 100644 --- a/docs/docs/internals/syntax.md +++ b/docs/docs/internals/syntax.md @@ -196,9 +196,10 @@ Expr1 ::= ‘if’ ‘(’ Expr ‘)’ {nl} | ForExpr | [SimpleExpr ‘.’] id ‘=’ Expr Assign(expr, expr) | SimpleExpr1 ArgumentExprs ‘=’ Expr Assign(expr, expr) - | PostfixExpr [Ascription] - | [‘inline’] PostfixExpr ‘match’ ‘{’ CaseClauses ‘}’ Match(expr, cases) -- point on match + | Expr2 + | [‘inline’] Expr2 ‘match’ ‘{’ CaseClauses ‘}’ Match(expr, cases) -- point on match | ‘implicit’ ‘match’ ‘{’ ImplicitCaseClauses ‘}’ +Expr2 ::= PostfixExpr [Ascription] Ascription ::= ‘:’ InfixType Typed(expr, tp) | ‘:’ Annotation {Annotation} Typed(expr, Annotated(EmptyTree, annot)*) Catches ::= ‘catch’ Expr @@ -224,7 +225,7 @@ SimpleExpr1 ::= Literal Quoted ::= ‘'’ ‘{’ Block ‘}’ | ‘'’ ‘[’ Type ‘]’ ExprsInParens ::= ExprInParens {‘,’ ExprInParens} -ExprInParens ::= PostfixExpr ‘:’ Type +ExprInParens ::= PostfixExpr ‘:’ Type -- normal Expr allows only RefinedType here | Expr ParArgumentExprs ::= ‘(’ ExprsInParens ‘)’ exprs | ‘(’ [ExprsInParens ‘,’] PostfixExpr ‘:’ ‘_’ ‘*’ ‘)’ exprs :+ Typed(expr, Ident(wildcardStar)) diff --git a/tests/pos-special/fatal-warnings/unchecked-scrutinee.scala b/tests/pos-special/fatal-warnings/unchecked-scrutinee.scala new file mode 100644 index 000000000000..a51a833f93da --- /dev/null +++ b/tests/pos-special/fatal-warnings/unchecked-scrutinee.scala @@ -0,0 +1,5 @@ +object Test { + List(1: @unchecked, 2, 3): @unchecked match { + case a :: as => + } +} \ No newline at end of file From 18b46a7a0e1fffa5969072873ac672058f1b7310 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 28 Apr 2019 16:46:16 +0200 Subject: [PATCH 02/12] Syntax change: multiple patterns in definition Allow `val a, b, c = ...` only if a, b, c are identifiers. It looks weird if they are general patterns, and it seems this gives unnecessary power. --- .../src/dotty/tools/dotc/parsing/Parsers.scala | 18 +++++++++++++----- docs/docs/internals/syntax.md | 3 ++- tests/neg/multi-patterns.scala | 4 ++++ tests/pos/i3412.scala | 2 -- 4 files changed, 19 insertions(+), 8 deletions(-) create mode 100644 tests/neg/multi-patterns.scala diff --git a/compiler/src/dotty/tools/dotc/parsing/Parsers.scala b/compiler/src/dotty/tools/dotc/parsing/Parsers.scala index 3751b3f869d0..06873f854b59 100644 --- a/compiler/src/dotty/tools/dotc/parsing/Parsers.scala +++ b/compiler/src/dotty/tools/dotc/parsing/Parsers.scala @@ -2355,13 +2355,21 @@ object Parsers { tmplDef(start, mods) } - /** PatDef ::= Pattern2 {`,' Pattern2} [`:' Type] `=' Expr - * VarDef ::= PatDef | id {`,' id} `:' Type `=' `_' - * ValDcl ::= id {`,' id} `:' Type - * VarDcl ::= id {`,' id} `:' Type + /** PatDef ::= ids [‘:’ Type] ‘=’ Expr + * | Pattern2 [‘:’ Type] ‘=’ Expr + * VarDef ::= PatDef | id {`,' id} `:' Type `=' `_' + * ValDcl ::= id {`,' id} `:' Type + * VarDcl ::= id {`,' id} `:' Type */ def patDefOrDcl(start: Offset, mods: Modifiers): Tree = atSpan(start, nameStart) { - val lhs = commaSeparated(pattern2) + val first = pattern2() + val lhs = first match { + case id: Ident if in.token == COMMA => + in.nextToken() + id :: commaSeparated(() => termIdent()) + case _ => + first :: Nil + } val tpt = typedOpt() val rhs = if (tpt.isEmpty || in.token == EQUALS) { diff --git a/docs/docs/internals/syntax.md b/docs/docs/internals/syntax.md index 17a0aebceeb4..e7e8a3cd4cd9 100644 --- a/docs/docs/internals/syntax.md +++ b/docs/docs/internals/syntax.md @@ -359,7 +359,8 @@ Def ::= ‘val’ PatDef | ‘type’ {nl} TypeDcl | TmplDef | INT -PatDef ::= Pattern2 {‘,’ Pattern2} [‘:’ Type] ‘=’ Expr PatDef(_, pats, tpe?, expr) +PatDef ::= ids [‘:’ Type] ‘=’ Expr + | Pattern2 [‘:’ Type] ‘=’ Expr PatDef(_, pats, tpe?, expr) VarDef ::= PatDef | ids ‘:’ Type ‘=’ ‘_’ DefDef ::= DefSig [(‘:’ | ‘<:’) Type] ‘=’ Expr DefDef(_, name, tparams, vparamss, tpe, expr) diff --git a/tests/neg/multi-patterns.scala b/tests/neg/multi-patterns.scala new file mode 100644 index 000000000000..2aabdf3f5b0b --- /dev/null +++ b/tests/neg/multi-patterns.scala @@ -0,0 +1,4 @@ +object Test { + val (a :: as), bs = List(1, 2, 3) // error + val B @ List(), C: List[Int] = List() // error +} \ No newline at end of file diff --git a/tests/pos/i3412.scala b/tests/pos/i3412.scala index 29c2bfd7690a..ff21986d3143 100644 --- a/tests/pos/i3412.scala +++ b/tests/pos/i3412.scala @@ -1,5 +1,3 @@ class Test { val A @ List() = List() - val B @ List(), C: List[Int] = List() - val D @ List(), E @ List() = List() } From f06281a7470d93d9ad9ffa6638b1a05679a52e68 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 28 Apr 2019 17:47:18 +0200 Subject: [PATCH 03/12] Allow ascriptions in pattern definitions --- .../dotty/tools/dotc/parsing/Parsers.scala | 27 ++++++++++++++----- docs/docs/internals/syntax.md | 2 +- tests/neg/unchecked-patterns.scala | 4 +++ tests/run/unchecked-patterns.scala | 8 ++++++ 4 files changed, 33 insertions(+), 8 deletions(-) create mode 100644 tests/neg/unchecked-patterns.scala create mode 100644 tests/run/unchecked-patterns.scala diff --git a/compiler/src/dotty/tools/dotc/parsing/Parsers.scala b/compiler/src/dotty/tools/dotc/parsing/Parsers.scala index 06873f854b59..07e284e3850a 100644 --- a/compiler/src/dotty/tools/dotc/parsing/Parsers.scala +++ b/compiler/src/dotty/tools/dotc/parsing/Parsers.scala @@ -1325,6 +1325,7 @@ object Parsers { t } case COLON => + in.nextToken() val t1 = ascription(t, location) if (in.token == MATCH) expr1Rest(t1, location) else t1 case MATCH => @@ -1334,7 +1335,6 @@ object Parsers { } def ascription(t: Tree, location: Location.Value): Tree = atSpan(startOffset(t)) { - in.skipToken() in.token match { case USCORE => val uscoreStart = in.skipToken() @@ -1803,7 +1803,10 @@ object Parsers { */ def pattern1(): Tree = { val p = pattern2() - if (isVarPattern(p) && in.token == COLON) ascription(p, Location.InPattern) + if (isVarPattern(p) && in.token == COLON) { + in.nextToken() + ascription(p, Location.InPattern) + } else p } @@ -2356,21 +2359,31 @@ object Parsers { } /** PatDef ::= ids [‘:’ Type] ‘=’ Expr - * | Pattern2 [‘:’ Type] ‘=’ Expr + * | Pattern2 [‘:’ Type | Ascription] ‘=’ Expr * VarDef ::= PatDef | id {`,' id} `:' Type `=' `_' * ValDcl ::= id {`,' id} `:' Type * VarDcl ::= id {`,' id} `:' Type */ def patDefOrDcl(start: Offset, mods: Modifiers): Tree = atSpan(start, nameStart) { val first = pattern2() - val lhs = first match { + var lhs = first match { case id: Ident if in.token == COMMA => in.nextToken() id :: commaSeparated(() => termIdent()) case _ => first :: Nil } - val tpt = typedOpt() + def emptyType = TypeTree().withSpan(Span(in.lastOffset)) + val tpt = + if (in.token == COLON) { + in.nextToken() + if (in.token == AT && lhs.tail.isEmpty) { + lhs = ascription(first, Location.ElseWhere) :: Nil + emptyType + } + else toplevelTyp() + } + else emptyType val rhs = if (tpt.isEmpty || in.token == EQUALS) { accept(EQUALS) @@ -2384,9 +2397,9 @@ object Parsers { lhs match { case (id: BackquotedIdent) :: Nil if id.name.isTermName => finalizeDef(BackquotedValDef(id.name.asTermName, tpt, rhs), mods, start) - case Ident(name: TermName) :: Nil => { + case Ident(name: TermName) :: Nil => finalizeDef(ValDef(name, tpt, rhs), mods, start) - } case _ => + case _ => PatDef(mods, lhs, tpt, rhs) } } diff --git a/docs/docs/internals/syntax.md b/docs/docs/internals/syntax.md index e7e8a3cd4cd9..7c071a2c9169 100644 --- a/docs/docs/internals/syntax.md +++ b/docs/docs/internals/syntax.md @@ -360,7 +360,7 @@ Def ::= ‘val’ PatDef | TmplDef | INT PatDef ::= ids [‘:’ Type] ‘=’ Expr - | Pattern2 [‘:’ Type] ‘=’ Expr PatDef(_, pats, tpe?, expr) + | Pattern2 [‘:’ Type | Ascription] ‘=’ Expr PatDef(_, pats, tpe?, expr) VarDef ::= PatDef | ids ‘:’ Type ‘=’ ‘_’ DefDef ::= DefSig [(‘:’ | ‘<:’) Type] ‘=’ Expr DefDef(_, name, tparams, vparamss, tpe, expr) diff --git a/tests/neg/unchecked-patterns.scala b/tests/neg/unchecked-patterns.scala new file mode 100644 index 000000000000..883d24bf7366 --- /dev/null +++ b/tests/neg/unchecked-patterns.scala @@ -0,0 +1,4 @@ +object Test { + val (y1: Some[Int] @unchecked) = Some(1): Option[Int] // OK + val y2: Some[Int] @unchecked = Some(1): Option[Int] // error +} \ No newline at end of file diff --git a/tests/run/unchecked-patterns.scala b/tests/run/unchecked-patterns.scala new file mode 100644 index 000000000000..0b9369219e64 --- /dev/null +++ b/tests/run/unchecked-patterns.scala @@ -0,0 +1,8 @@ +object Test extends App { + val x: Int @unchecked = 2 + val (y1: Some[Int] @unchecked) = Some(1): Option[Int] + + val a :: as: @unchecked = List(1, 2, 3) + + +} \ No newline at end of file From c6a7e8c0e24a297c938c3b95490d73f34ab16c98 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 28 Apr 2019 20:21:54 +0200 Subject: [PATCH 04/12] Fix replTest --- compiler/test-resources/repl/patdef | 3 --- 1 file changed, 3 deletions(-) diff --git a/compiler/test-resources/repl/patdef b/compiler/test-resources/repl/patdef index 9fab65d2ae7a..12ba13bcd6d5 100644 --- a/compiler/test-resources/repl/patdef +++ b/compiler/test-resources/repl/patdef @@ -21,6 +21,3 @@ scala> val _ @ List(x) = List(1) val x: Int = 1 scala> val List(_ @ List(x)) = List(List(2)) val x: Int = 2 -scala> val B @ List(), C: List[Int] = List() -val B: List[Int] = List() -val C: List[Int] = List() From ce35deb37ab7bb91ce973cfaf6f8f956563b6808 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 28 Apr 2019 21:17:56 +0200 Subject: [PATCH 05/12] Error on narrowing pattern definitions --- tests/neg/unchecked-patterns.scala | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/neg/unchecked-patterns.scala b/tests/neg/unchecked-patterns.scala index 883d24bf7366..a0fcdfebdc7e 100644 --- a/tests/neg/unchecked-patterns.scala +++ b/tests/neg/unchecked-patterns.scala @@ -1,4 +1,6 @@ object Test { val (y1: Some[Int] @unchecked) = Some(1): Option[Int] // OK val y2: Some[Int] @unchecked = Some(1): Option[Int] // error + + val x :: xs = List(1, 2, 3) // error } \ No newline at end of file From 43623758de1ee983fa7608aba062e165909d737b Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 28 Apr 2019 21:56:20 +0200 Subject: [PATCH 06/12] Treat narrowing pattern definitions as errors --- .../src/dotty/tools/dotc/ast/Desugar.scala | 9 +++++++- .../src/dotty/tools/dotc/typer/Checking.scala | 21 +++++++++++++++++++ .../tools/dotc/typer/ErrorReporting.scala | 4 ++++ .../src/dotty/tools/dotc/typer/Typer.scala | 5 ++++- tests/run/unchecked-patterns.scala | 2 -- 5 files changed, 37 insertions(+), 4 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/ast/Desugar.scala b/compiler/src/dotty/tools/dotc/ast/Desugar.scala index fea57a20dce0..5d8725d77b2d 100644 --- a/compiler/src/dotty/tools/dotc/ast/Desugar.scala +++ b/compiler/src/dotty/tools/dotc/ast/Desugar.scala @@ -32,6 +32,9 @@ object desugar { */ val DerivingCompanion: Property.Key[SourcePosition] = new Property.Key + /** An attachment for match expressions generated from a PatDef */ + val PatDefMatch: Property.Key[Unit] = new Property.Key + /** Info of a variable in a pattern: The named tree and its type */ private type VarInfo = (NameTree, Tree) @@ -956,7 +959,11 @@ object desugar { // - `pat` is a tuple of N variables or wildcard patterns like `(x1, x2, ..., xN)` val tupleOptimizable = forallResults(rhs, isMatchingTuple) - def rhsUnchecked = makeAnnotated("scala.unchecked", rhs) + def rhsUnchecked = { + val rhs1 = makeAnnotated("scala.unchecked", rhs) + rhs1.pushAttachment(PatDefMatch, ()) + rhs1 + } val vars = if (tupleOptimizable) // include `_` pat match { diff --git a/compiler/src/dotty/tools/dotc/typer/Checking.scala b/compiler/src/dotty/tools/dotc/typer/Checking.scala index ce6431e2bc12..955929092651 100644 --- a/compiler/src/dotty/tools/dotc/typer/Checking.scala +++ b/compiler/src/dotty/tools/dotc/typer/Checking.scala @@ -16,6 +16,8 @@ import ProtoTypes._ import Scopes._ import CheckRealizable._ import ErrorReporting.errorTree +import rewrites.Rewrites.patch +import util.Spans.Span import util.SourcePosition import transform.SymUtils._ @@ -594,6 +596,25 @@ trait Checking { ctx.error(ex"$cls cannot be instantiated since it${rstatus.msg}", pos) } + /** Check that pattern definition is either marked @unchecked or has a right + * hand side with a type that conforms to the pattern's type. + */ + def checkPatDefMatch(tree: Tree, pt: Type)(implicit ctx: Context): Unit = tree match { + case Match(_, CaseDef(pat, _, _) :: _) + if !pat.tpe.widen.hasAnnotation(defn.UncheckedAnnot) && !(pt <:< pat.tpe) => + val pt1 = pt match { + case AnnotatedType(pt1, annot) if annot.matches(defn.UncheckedAnnot) => pt1 + case _ => pt + } + ctx.errorOrMigrationWarning( + ex"""pattern's type ${pat.tpe.widen} is more specialized than the right hand side expression's type $pt1 + | + |If the narrowing is intentional, this can be communicated by writing `: @unchecked` after the pattern.${err.rewriteNotice}""", + pat.sourcePos) + if (ctx.scala2Mode) patch(Span(pat.span.end), ": @unchecked") + case _ => + } + /** Check that `path` is a legal prefix for an import or export clause */ def checkLegalImportPath(path: Tree)(implicit ctx: Context): Unit = { checkStable(path.tpe, path.sourcePos) diff --git a/compiler/src/dotty/tools/dotc/typer/ErrorReporting.scala b/compiler/src/dotty/tools/dotc/typer/ErrorReporting.scala index 03b67a7a91fe..05f785bbe95f 100644 --- a/compiler/src/dotty/tools/dotc/typer/ErrorReporting.scala +++ b/compiler/src/dotty/tools/dotc/typer/ErrorReporting.scala @@ -157,6 +157,10 @@ object ErrorReporting { } """\$\{\w*\}""".r.replaceSomeIn(raw, m => translate(m.matched.drop(2).init)) } + + def rewriteNotice: String = + if (ctx.scala2Mode) "\nThis patch can be inserted automatically under -rewrite." + else "" } def err(implicit ctx: Context): Errors = new Errors diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala index f07563ec49f3..278e37df9012 100644 --- a/compiler/src/dotty/tools/dotc/typer/Typer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Typer.scala @@ -1036,7 +1036,10 @@ class Typer extends Namer if (tree.isInline) checkInInlineContext("inline match", tree.posd) val sel1 = typedExpr(tree.selector) val selType = fullyDefinedType(sel1.tpe, "pattern selector", tree.span).widen - typedMatchFinish(tree, sel1, selType, tree.cases, pt) + val res = typedMatchFinish(tree, sel1, selType, tree.cases, pt) + if (tree.selector.removeAttachment(desugar.PatDefMatch).isDefined) + checkPatDefMatch(res, sel1.tpe) + res } } diff --git a/tests/run/unchecked-patterns.scala b/tests/run/unchecked-patterns.scala index 0b9369219e64..c650fc24d58b 100644 --- a/tests/run/unchecked-patterns.scala +++ b/tests/run/unchecked-patterns.scala @@ -3,6 +3,4 @@ object Test extends App { val (y1: Some[Int] @unchecked) = Some(1): Option[Int] val a :: as: @unchecked = List(1, 2, 3) - - } \ No newline at end of file From 9bc39e61cf8e3b32198b6347df63d887c3123dd3 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 29 Apr 2019 19:43:16 +0200 Subject: [PATCH 07/12] Check that patterns in val/var defs are irrefutable ... unless followed by `: @unchecked`. For the moment, this requires -strict mode. We plan to turn this on by default once bootstrap compiler supports `: @unchecked` in patterns (i.e. once this PR is part of the bootstrap compiler). --- .../src/dotty/tools/dotc/core/Types.scala | 14 +++-- .../dotty/tools/dotc/typer/Applications.scala | 2 +- .../src/dotty/tools/dotc/typer/Checking.scala | 54 +++++++++++++------ .../src/dotty/tools/dotc/typer/Typer.scala | 20 ++++--- tests/neg/unchecked-patterns.scala | 6 ++- tests/run/unchecked-patterns.scala | 3 ++ 6 files changed, 71 insertions(+), 28 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/Types.scala b/compiler/src/dotty/tools/dotc/core/Types.scala index 49908bacdcd6..2358d0c73e53 100644 --- a/compiler/src/dotty/tools/dotc/core/Types.scala +++ b/compiler/src/dotty/tools/dotc/core/Types.scala @@ -1528,13 +1528,17 @@ object Types { */ def signature(implicit ctx: Context): Signature = Signature.NotAMethod - def dropRepeatedAnnot(implicit ctx: Context): Type = this match { - case AnnotatedType(parent, annot) if annot.symbol eq defn.RepeatedAnnot => parent - case tp @ AnnotatedType(parent, annot) => - tp.derivedAnnotatedType(parent.dropRepeatedAnnot, annot) - case tp => tp + /** Drop annotation of given `cls` from this type */ + def dropAnnot(cls: Symbol)(implicit ctx: Context): Type = stripTypeVar match { + case self @ AnnotatedType(pre, annot) => + if (annot.symbol eq cls) pre + else self.derivedAnnotatedType(pre.dropAnnot(cls), annot) + case _ => + this } + def dropRepeatedAnnot(implicit ctx: Context): Type = dropAnnot(defn.RepeatedAnnot) + def annotatedToRepeated(implicit ctx: Context): Type = this match { case tp @ ExprType(tp1) => tp.derivedExprType(tp1.annotatedToRepeated) case AnnotatedType(tp, annot) if annot matches defn.RepeatedAnnot => diff --git a/compiler/src/dotty/tools/dotc/typer/Applications.scala b/compiler/src/dotty/tools/dotc/typer/Applications.scala index ca5d94dba23b..45eac2ea9bf3 100644 --- a/compiler/src/dotty/tools/dotc/typer/Applications.scala +++ b/compiler/src/dotty/tools/dotc/typer/Applications.scala @@ -1105,7 +1105,7 @@ trait Applications extends Compatibility { self: Typer with Dynamic => if (selType <:< unapplyArgType) { unapp.println(i"case 1 $unapplyArgType ${ctx.typerState.constraint}") fullyDefinedType(unapplyArgType, "pattern selector", tree.span) - selType + selType.dropAnnot(defn.UncheckedAnnot) // need to drop @unchecked. Just because the selector is @unchecked, the pattern isn't. } else if (isSubTypeOfParent(unapplyArgType, selType)(ctx.addMode(Mode.GADTflexible))) { val patternBound = maximizeType(unapplyArgType, tree.span, fromScala2x) if (patternBound.nonEmpty) unapplyFn = addBinders(unapplyFn, patternBound) diff --git a/compiler/src/dotty/tools/dotc/typer/Checking.scala b/compiler/src/dotty/tools/dotc/typer/Checking.scala index 955929092651..b251a5c2690d 100644 --- a/compiler/src/dotty/tools/dotc/typer/Checking.scala +++ b/compiler/src/dotty/tools/dotc/typer/Checking.scala @@ -25,6 +25,7 @@ import Decorators._ import ErrorReporting.{err, errorType} import config.Printers.typr import NameKinds.DefaultGetterName +import Applications.unapplyArgs import collection.mutable import SymDenotations.{NoCompleter, NoDenotation} @@ -596,23 +597,46 @@ trait Checking { ctx.error(ex"$cls cannot be instantiated since it${rstatus.msg}", pos) } - /** Check that pattern definition is either marked @unchecked or has a right - * hand side with a type that conforms to the pattern's type. + /** Check that pattern `pat` is irrefutable for scrutinee tye `pt`. + * This means `pat` is either marked @unchecked or `pt` conforms to the + * pattern's type. If pattern is an UnApply, do the check recursively. */ - def checkPatDefMatch(tree: Tree, pt: Type)(implicit ctx: Context): Unit = tree match { - case Match(_, CaseDef(pat, _, _) :: _) - if !pat.tpe.widen.hasAnnotation(defn.UncheckedAnnot) && !(pt <:< pat.tpe) => - val pt1 = pt match { - case AnnotatedType(pt1, annot) if annot.matches(defn.UncheckedAnnot) => pt1 - case _ => pt + def checkIrrefutable(pat: Tree, pt: Type)(implicit ctx: Context): Boolean = { + patmatch.println(i"check irrefutable $pat: ${pat.tpe} against $pt") + + def check(pat: Tree, pt: Type): Boolean = { + if (pt <:< pat.tpe) + true + else { + ctx.errorOrMigrationWarning( + ex"""pattern's type ${pat.tpe} is more specialized than the right hand side expression's type ${pt.dropAnnot(defn.UncheckedAnnot)} + | + |If the narrowing is intentional, this can be communicated by writing `: @unchecked` after the full pattern.${err.rewriteNotice}""", + pat.sourcePos) + false + } + } + + !ctx.settings.strict.value || // only in -strict mode for now since mitigations work only after this PR + pat.tpe.widen.hasAnnotation(defn.UncheckedAnnot) || { + pat match { + case Bind(_, pat1) => + checkIrrefutable(pat1, pt) + case UnApply(fn, _, pats) => + check(pat, pt) && { + val argPts = unapplyArgs(fn.tpe.finalResultType, fn, pats, pat.sourcePos) + pats.corresponds(argPts)(checkIrrefutable) + } + case Alternative(pats) => + pats.forall(checkIrrefutable(_, pt)) + case Typed(arg, tpt) => + check(pat, pt) && checkIrrefutable(arg, pt) + case Ident(nme.WILDCARD) => + true + case _ => + check(pat, pt) } - ctx.errorOrMigrationWarning( - ex"""pattern's type ${pat.tpe.widen} is more specialized than the right hand side expression's type $pt1 - | - |If the narrowing is intentional, this can be communicated by writing `: @unchecked` after the pattern.${err.rewriteNotice}""", - pat.sourcePos) - if (ctx.scala2Mode) patch(Span(pat.span.end), ": @unchecked") - case _ => + } } /** Check that `path` is a legal prefix for an import or export clause */ diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala index 278e37df9012..7474f6bb0261 100644 --- a/compiler/src/dotty/tools/dotc/typer/Typer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Typer.scala @@ -1036,10 +1036,15 @@ class Typer extends Namer if (tree.isInline) checkInInlineContext("inline match", tree.posd) val sel1 = typedExpr(tree.selector) val selType = fullyDefinedType(sel1.tpe, "pattern selector", tree.span).widen - val res = typedMatchFinish(tree, sel1, selType, tree.cases, pt) - if (tree.selector.removeAttachment(desugar.PatDefMatch).isDefined) - checkPatDefMatch(res, sel1.tpe) - res + val result = typedMatchFinish(tree, sel1, selType, tree.cases, pt) + result match { + case Match(sel, CaseDef(pat, _, _) :: _) + if (tree.selector.removeAttachment(desugar.PatDefMatch).isDefined) => + if (!checkIrrefutable(pat, sel.tpe) && ctx.scala2Mode) + patch(Span(pat.span.end), ": @unchecked") + case _ => + } + result } } @@ -1820,8 +1825,11 @@ class Typer extends Namer } case _ => arg1 } - val tpt = TypeTree(AnnotatedType(arg1.tpe.widenIfUnstable, Annotation(annot1))) - assignType(cpy.Typed(tree)(arg2, tpt), tpt) + val argType = + if (arg1.isInstanceOf[Bind]) arg1.tpe.widen // bound symbol is not accessible outside of Bind node + else arg1.tpe.widenIfUnstable + val annotatedTpt = TypeTree(AnnotatedType(argType, Annotation(annot1))) + assignType(cpy.Typed(tree)(arg2, annotatedTpt), annotatedTpt) } } diff --git a/tests/neg/unchecked-patterns.scala b/tests/neg/unchecked-patterns.scala index a0fcdfebdc7e..ada7680491ed 100644 --- a/tests/neg/unchecked-patterns.scala +++ b/tests/neg/unchecked-patterns.scala @@ -2,5 +2,9 @@ object Test { val (y1: Some[Int] @unchecked) = Some(1): Option[Int] // OK val y2: Some[Int] @unchecked = Some(1): Option[Int] // error - val x :: xs = List(1, 2, 3) // error + val x :: xs = List(1, 2, 3) // error + val (1, c) = (1, 2) // error + val 1 *: cs = 1 *: () // error + + val (_: Int | _: Any) = ??? : Any // error } \ No newline at end of file diff --git a/tests/run/unchecked-patterns.scala b/tests/run/unchecked-patterns.scala index c650fc24d58b..03e9cd510fb0 100644 --- a/tests/run/unchecked-patterns.scala +++ b/tests/run/unchecked-patterns.scala @@ -3,4 +3,7 @@ object Test extends App { val (y1: Some[Int] @unchecked) = Some(1): Option[Int] val a :: as: @unchecked = List(1, 2, 3) + val lst @ b :: bs: @unchecked = List(1, 2, 3) + val (1, c): @unchecked = (1, 2) + val 1 *: cs: @unchecked = 1 *: () // error } \ No newline at end of file From ec5c96051986949bda4c0563b4751c84fb8c0d8d Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 29 Apr 2019 21:20:58 +0200 Subject: [PATCH 08/12] Add missing import --- compiler/src/dotty/tools/dotc/typer/Checking.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/src/dotty/tools/dotc/typer/Checking.scala b/compiler/src/dotty/tools/dotc/typer/Checking.scala index b251a5c2690d..78df00030620 100644 --- a/compiler/src/dotty/tools/dotc/typer/Checking.scala +++ b/compiler/src/dotty/tools/dotc/typer/Checking.scala @@ -23,7 +23,7 @@ import util.SourcePosition import transform.SymUtils._ import Decorators._ import ErrorReporting.{err, errorType} -import config.Printers.typr +import config.Printers.{typr, patmatch} import NameKinds.DefaultGetterName import Applications.unapplyArgs From 21a5bbec748aa309da87b955f3aa08354b586427 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sat, 4 May 2019 11:11:22 +0200 Subject: [PATCH 09/12] Fix -strict tests - move all strict neg tests to a special directory - disable -strict for library bootstrap. tasty.reflect sources fail under -strict, but this can be fixed only when the original bootstrap compiler accepts : @unchecked in pattern definitions. --- compiler/test/dotty/tools/dotc/CompilationTests.scala | 7 ++++--- tests/{neg-custom-args => neg-strict}/i1050.scala | 0 tests/{neg-custom-args => neg-strict}/nullless.scala | 0 tests/{neg => neg-strict}/unchecked-patterns.scala | 0 4 files changed, 4 insertions(+), 3 deletions(-) rename tests/{neg-custom-args => neg-strict}/i1050.scala (100%) rename tests/{neg-custom-args => neg-strict}/nullless.scala (100%) rename tests/{neg => neg-strict}/unchecked-patterns.scala (100%) diff --git a/compiler/test/dotty/tools/dotc/CompilationTests.scala b/compiler/test/dotty/tools/dotc/CompilationTests.scala index 4bb3ebce93b8..9ab3cb81be24 100644 --- a/compiler/test/dotty/tools/dotc/CompilationTests.scala +++ b/compiler/test/dotty/tools/dotc/CompilationTests.scala @@ -150,6 +150,7 @@ class CompilationTests extends ParallelTesting { aggregateTests( compileFilesInDir("tests/neg", defaultOptions), compileFilesInDir("tests/neg-tailcall", defaultOptions), + compileFilesInDir("tests/neg-strict", defaultOptions.and("-strict")), compileFilesInDir("tests/neg-no-kind-polymorphism", defaultOptions and "-Yno-kind-polymorphism"), compileFilesInDir("tests/neg-custom-args/deprecation", defaultOptions.and("-Xfatal-warnings", "-deprecation")), compileFilesInDir("tests/neg-custom-args/fatal-warnings", defaultOptions.and("-Xfatal-warnings")), @@ -160,8 +161,6 @@ class CompilationTests extends ParallelTesting { compileFile("tests/neg-custom-args/i3246.scala", scala2Mode), compileFile("tests/neg-custom-args/overrideClass.scala", scala2Mode), compileFile("tests/neg-custom-args/autoTuplingTest.scala", defaultOptions.and("-language:noAutoTupling")), - compileFile("tests/neg-custom-args/i1050.scala", defaultOptions.and("-strict")), - compileFile("tests/neg-custom-args/nullless.scala", defaultOptions.and("-strict")), compileFile("tests/neg-custom-args/nopredef.scala", defaultOptions.and("-Yno-predef")), compileFile("tests/neg-custom-args/noimports.scala", defaultOptions.and("-Yno-imports")), compileFile("tests/neg-custom-args/noimports2.scala", defaultOptions.and("-Yno-imports")), @@ -249,7 +248,9 @@ class CompilationTests extends ParallelTesting { val lib = compileList("src", librarySources, - defaultOptions.and("-Ycheck-reentrant", "-strict", "-priorityclasspath", defaultOutputDir))(libGroup) + defaultOptions.and("-Ycheck-reentrant", + // "-strict", // TODO: re-enable once we allow : @unchecked in pattern definitions. Right now, lots of narrowing pattern definitions fail. + "-priorityclasspath", defaultOutputDir))(libGroup) val compilerSources = sources(Paths.get("compiler/src")) val compilerManagedSources = sources(Properties.dottyCompilerManagedSources) diff --git a/tests/neg-custom-args/i1050.scala b/tests/neg-strict/i1050.scala similarity index 100% rename from tests/neg-custom-args/i1050.scala rename to tests/neg-strict/i1050.scala diff --git a/tests/neg-custom-args/nullless.scala b/tests/neg-strict/nullless.scala similarity index 100% rename from tests/neg-custom-args/nullless.scala rename to tests/neg-strict/nullless.scala diff --git a/tests/neg/unchecked-patterns.scala b/tests/neg-strict/unchecked-patterns.scala similarity index 100% rename from tests/neg/unchecked-patterns.scala rename to tests/neg-strict/unchecked-patterns.scala From e127bb4b3d48cd6bfa68b6e159d1fdb627578a97 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 7 May 2019 12:34:08 +0200 Subject: [PATCH 10/12] Fix treatment of UnApply pattern definitions --- .../tools/dotc/transform/patmat/Space.scala | 31 +++++++++++-------- .../src/dotty/tools/dotc/typer/Checking.scala | 26 ++++++++-------- tests/neg-strict/unchecked-patterns.scala | 13 ++++++++ tests/run/unchecked-patterns.scala | 4 ++- 4 files changed, 47 insertions(+), 27 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala b/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala index 06fcaa907870..62f7346c748c 100644 --- a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala +++ b/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala @@ -280,11 +280,25 @@ trait SpaceLogic { } } +object SpaceEngine { + + /** Is the unapply irrefutable? + * @param unapp The unapply function reference + */ + def isIrrefutableUnapply(unapp: tpd.Tree)(implicit ctx: Context): Boolean = { + val unappResult = unapp.tpe.widen.finalResultType + unappResult.isRef(defn.SomeClass) || + unappResult =:= ConstantType(Constant(true)) || + (unapp.symbol.is(Synthetic) && unapp.symbol.owner.linkedClass.is(Case)) || + productArity(unappResult) > 0 + } +} + /** Scala implementation of space logic */ class SpaceEngine(implicit ctx: Context) extends SpaceLogic { import tpd._ + import SpaceEngine._ - private val scalaSomeClass = ctx.requiredClass("scala.Some") private val scalaSeqFactoryClass = ctx.requiredClass("scala.collection.generic.SeqFactory") private val scalaListType = ctx.requiredClassRef("scala.collection.immutable.List") private val scalaNilType = ctx.requiredModuleRef("scala.collection.immutable.Nil") @@ -309,15 +323,6 @@ class SpaceEngine(implicit ctx: Context) extends SpaceLogic { else Typ(AndType(tp1, tp2), true) } - /** Whether the extractor is irrefutable */ - def irrefutable(unapp: Tree): Boolean = { - // TODO: optionless patmat - unapp.tpe.widen.finalResultType.isRef(scalaSomeClass) || - unapp.tpe.widen.finalResultType =:= ConstantType(Constant(true)) || - (unapp.symbol.is(Synthetic) && unapp.symbol.owner.linkedClass.is(Case)) || - productArity(unapp.tpe.widen.finalResultType) > 0 - } - /** Return the space that represents the pattern `pat` */ def project(pat: Tree): Space = pat match { case Literal(c) => @@ -340,12 +345,12 @@ class SpaceEngine(implicit ctx: Context) extends SpaceLogic { else { val (arity, elemTp, resultTp) = unapplySeqInfo(fun.tpe.widen.finalResultType, fun.sourcePos) if (elemTp.exists) - Prod(erase(pat.tpe.stripAnnots), fun.tpe, fun.symbol, projectSeq(pats) :: Nil, irrefutable(fun)) + Prod(erase(pat.tpe.stripAnnots), fun.tpe, fun.symbol, projectSeq(pats) :: Nil, isIrrefutableUnapply(fun)) else - Prod(erase(pat.tpe.stripAnnots), fun.tpe, fun.symbol, pats.take(arity - 1).map(project) :+ projectSeq(pats.drop(arity - 1)), irrefutable(fun)) + Prod(erase(pat.tpe.stripAnnots), fun.tpe, fun.symbol, pats.take(arity - 1).map(project) :+ projectSeq(pats.drop(arity - 1)),isIrrefutableUnapply(fun)) } else - Prod(erase(pat.tpe.stripAnnots), fun.tpe, fun.symbol, pats.map(project), irrefutable(fun)) + Prod(erase(pat.tpe.stripAnnots), fun.tpe, fun.symbol, pats.map(project), isIrrefutableUnapply(fun)) case Typed(pat @ UnApply(_, _, _), _) => project(pat) case Typed(expr, tpt) => Typ(erase(expr.tpe.stripAnnots), true) diff --git a/compiler/src/dotty/tools/dotc/typer/Checking.scala b/compiler/src/dotty/tools/dotc/typer/Checking.scala index 78df00030620..6fc0de6b9cfa 100644 --- a/compiler/src/dotty/tools/dotc/typer/Checking.scala +++ b/compiler/src/dotty/tools/dotc/typer/Checking.scala @@ -26,6 +26,7 @@ import ErrorReporting.{err, errorType} import config.Printers.{typr, patmatch} import NameKinds.DefaultGetterName import Applications.unapplyArgs +import transform.patmat.SpaceEngine.isIrrefutableUnapply import collection.mutable import SymDenotations.{NoCompleter, NoDenotation} @@ -604,27 +605,26 @@ trait Checking { def checkIrrefutable(pat: Tree, pt: Type)(implicit ctx: Context): Boolean = { patmatch.println(i"check irrefutable $pat: ${pat.tpe} against $pt") - def check(pat: Tree, pt: Type): Boolean = { - if (pt <:< pat.tpe) - true - else { - ctx.errorOrMigrationWarning( - ex"""pattern's type ${pat.tpe} is more specialized than the right hand side expression's type ${pt.dropAnnot(defn.UncheckedAnnot)} - | - |If the narrowing is intentional, this can be communicated by writing `: @unchecked` after the full pattern.${err.rewriteNotice}""", - pat.sourcePos) - false - } + def fail(pat: Tree, pt: Type): Boolean = { + ctx.errorOrMigrationWarning( + ex"""pattern's type ${pat.tpe} is more specialized than the right hand side expression's type ${pt.dropAnnot(defn.UncheckedAnnot)} + | + |If the narrowing is intentional, this can be communicated by writing `: @unchecked` after the full pattern.${err.rewriteNotice}""", + pat.sourcePos) + false } + def check(pat: Tree, pt: Type): Boolean = (pt <:< pat.tpe) || fail(pat, pt) + !ctx.settings.strict.value || // only in -strict mode for now since mitigations work only after this PR pat.tpe.widen.hasAnnotation(defn.UncheckedAnnot) || { pat match { case Bind(_, pat1) => checkIrrefutable(pat1, pt) case UnApply(fn, _, pats) => - check(pat, pt) && { - val argPts = unapplyArgs(fn.tpe.finalResultType, fn, pats, pat.sourcePos) + check(pat, pt) && + (isIrrefutableUnapply(fn) || fail(pat, pt)) && { + val argPts = unapplyArgs(fn.tpe.widen.finalResultType, fn, pats, pat.sourcePos) pats.corresponds(argPts)(checkIrrefutable) } case Alternative(pats) => diff --git a/tests/neg-strict/unchecked-patterns.scala b/tests/neg-strict/unchecked-patterns.scala index ada7680491ed..d6a8cb70cc2a 100644 --- a/tests/neg-strict/unchecked-patterns.scala +++ b/tests/neg-strict/unchecked-patterns.scala @@ -1,4 +1,5 @@ object Test { + val (y1: Some[Int] @unchecked) = Some(1): Option[Int] // OK val y2: Some[Int] @unchecked = Some(1): Option[Int] // error @@ -7,4 +8,16 @@ object Test { val 1 *: cs = 1 *: () // error val (_: Int | _: Any) = ??? : Any // error + + object Positive { def unapply(i: Int): Option[Int] = Some(i).filter(_ > 0) } + object Always1 { def unapply(i: Int): Some[Int] = Some(i) } + object Pair { def unapply(t: (Int, Int)): t.type = t } + object Triple { def unapply(t: (Int, Int, Int)): (Int, Int, Int) = t } + + val Positive(p) = 5 // error + val Some(s1) = Option(1) // error + val Some(s2) = Some(1) // OK + val Always1(p1) = 5 // OK + val Pair(t1, t2) = (5, 5) // OK + val Triple(u1, u2, u3) = (5, 5, 5) // OK } \ No newline at end of file diff --git a/tests/run/unchecked-patterns.scala b/tests/run/unchecked-patterns.scala index 03e9cd510fb0..07539312d79f 100644 --- a/tests/run/unchecked-patterns.scala +++ b/tests/run/unchecked-patterns.scala @@ -5,5 +5,7 @@ object Test extends App { val a :: as: @unchecked = List(1, 2, 3) val lst @ b :: bs: @unchecked = List(1, 2, 3) val (1, c): @unchecked = (1, 2) - val 1 *: cs: @unchecked = 1 *: () // error + + object Positive { def unapply(i: Int): Option[Int] = Some(i).filter(_ > 0) } + val Positive(p): @unchecked = 5 } \ No newline at end of file From b95c66e24facb47d17f8f4f37407b5ffe3edee5d Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 7 May 2019 12:36:40 +0200 Subject: [PATCH 11/12] Avoid spurious type test warning This warning should trigger only for explicitly written type tests, never for type tests coming from a match. So far we did not notice this, since it triggered only for types that are known to be null, and there are very few of those so far (basically, just the primitive value types) and those types did not tend to be tested irrefutably in patterns. --- compiler/src/dotty/tools/dotc/transform/TypeTestsCasts.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/src/dotty/tools/dotc/transform/TypeTestsCasts.scala b/compiler/src/dotty/tools/dotc/transform/TypeTestsCasts.scala index 5a08e97366ec..89c841acdd40 100644 --- a/compiler/src/dotty/tools/dotc/transform/TypeTestsCasts.scala +++ b/compiler/src/dotty/tools/dotc/transform/TypeTestsCasts.scala @@ -215,7 +215,7 @@ object TypeTestsCasts { if (expr.tpe <:< testType) if (expr.tpe.isNotNull) { - ctx.warning(TypeTestAlwaysSucceeds(foundCls, testCls), tree.sourcePos) + if (!inMatch) ctx.warning(TypeTestAlwaysSucceeds(foundCls, testCls), tree.sourcePos) constant(expr, Literal(Constant(true))) } else expr.testNotNull From 80a3a67445c5be2272c8b844d982811befd00ed5 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 7 May 2019 13:56:00 +0200 Subject: [PATCH 12/12] Reclassify test I believe #4674 had the wrong resolution. The test leads to spurious warnings for compiler-generated pattern matches. I believe it's better to revert the decision and check only explicit isInstanceOf, never patterns. Note the scalac does not check patterns either. --- .../{neg-custom-args => pos-special}/fatal-warnings/i4674.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename tests/{neg-custom-args => pos-special}/fatal-warnings/i4674.scala (58%) diff --git a/tests/neg-custom-args/fatal-warnings/i4674.scala b/tests/pos-special/fatal-warnings/i4674.scala similarity index 58% rename from tests/neg-custom-args/fatal-warnings/i4674.scala rename to tests/pos-special/fatal-warnings/i4674.scala index 6595ee3f3ec2..53108d7981ca 100644 --- a/tests/neg-custom-args/fatal-warnings/i4674.scala +++ b/tests/pos-special/fatal-warnings/i4674.scala @@ -2,7 +2,7 @@ class Test { def test(x: String) = { x.foreach { case 's' => println("s") - case c: Char => println(c) // error: type test always succeeds + case c: Char => println(c) // should compile without warning } } }