Skip to content

Commit 1b5138d

Browse files
oderskyodersky
authored
Some usability improvements relating to errors (#23370)
The changes in this PR were motivated by a very nice live coding session of Nicolas Rinaudo at the Scala User's group meetup in Krakow. We identified 4 areas where the feed-back of the compiler could be clearer: 1. Trying to add a capture set to a pure type, such as in `Int^` should give an error. 2. Don't complain about inferred type not conforming to external type if the definition in question is in the empty package. Improve the error message for the remaining definitions. 4. Don't show expected types as dependent function types if they were generic function types originally 5. Don't print "box" unless we are in -Ycc-verbose mode.
2 parents a110500 + 523bc78 commit 1b5138d

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

67 files changed

+383
-354
lines changed

‎compiler/src/dotty/tools/dotc/cc/CaptureOps.scala‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -280,9 +280,9 @@ extension (tp: Type)
280280
case tp: (TypeRef | AppliedType) =>
281281
val sym = tp.typeSymbol
282282
if sym.isClass then sym.isPureClass
283-
else tp.superType.isAlwaysPure
283+
else !tp.superType.isAny && tp.superType.isAlwaysPure
284284
case tp: TypeProxy =>
285-
tp.superType.isAlwaysPure
285+
!tp.superType.isAny && tp.superType.isAlwaysPure
286286
case tp: AndType =>
287287
tp.tp1.isAlwaysPure || tp.tp2.isAlwaysPure
288288
case tp: OrType =>

‎compiler/src/dotty/tools/dotc/cc/CheckCaptures.scala‎

Lines changed: 36 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1059,14 +1059,24 @@ class CheckCaptures extends Recheck, SymTransformer:
10591059

10601060
def canUseInferred = // If canUseInferred is false, all capturing types in the type of `sym` need to be given explicitly
10611061
sym.isLocalToCompilationUnit // Symbols that can't be seen outside the compilation unit can always have inferred types
1062-
|| sym.privateWithin == defn.EmptyPackageClass
1063-
// We make an exception for private symbols in a toplevel file in the empty package
1062+
|| ctx.owner.enclosingPackageClass.isEmptyPackage
1063+
// We make an exception for symbols in the empty package.
10641064
// these could theoretically be accessed from other files in the empty package, but
1065-
// it would be too annoying to require explicit types.
1065+
// usually it would be too annoying to require explicit types.
10661066
|| sym.name.is(DefaultGetterName) // Default getters are exempted since otherwise it would be
10671067
// too annoying. This is a hole since a defualt getter's result type
10681068
// might leak into a type variable.
10691069

1070+
def fail(tree: Tree, expected: Type, addenda: Addenda): Unit =
1071+
def maybeResult = if sym.is(Method) then " result" else ""
1072+
report.error(
1073+
em"""$sym needs an explicit$maybeResult type because the inferred type does not conform to
1074+
|the type that is externally visible in other compilation units.
1075+
|
1076+
| Inferred type : ${tree.tpe}
1077+
| Externally visible type: $expected""",
1078+
tree.srcPos)
1079+
10701080
def addenda(expected: Type) = new Addenda:
10711081
override def toAdd(using Context) =
10721082
def result = if tree.isInstanceOf[ValDef] then"" else " result"
@@ -1083,7 +1093,7 @@ class CheckCaptures extends Recheck, SymTransformer:
10831093
val expected = tpt.tpe.dropAllRetains
10841094
todoAtPostCheck += { () =>
10851095
withCapAsRoot:
1086-
checkConformsExpr(tp, expected, tree.rhs, addenda(expected))
1096+
testAdapted(tp, expected, tree.rhs, addenda(expected))(fail)
10871097
// The check that inferred <: expected is done after recheck so that it
10881098
// does not interfere with normal rechecking by constraining capture set variables.
10891099
}
@@ -1322,7 +1332,12 @@ class CheckCaptures extends Recheck, SymTransformer:
13221332
* where local capture roots are instantiated to root variables.
13231333
*/
13241334
override def checkConformsExpr(actual: Type, expected: Type, tree: Tree, addenda: Addenda)(using Context): Type =
1335+
testAdapted(actual, expected, tree, addenda)(err.typeMismatch)
1336+
1337+
inline def testAdapted(actual: Type, expected: Type, tree: Tree, addenda: Addenda)
1338+
(fail: (Tree, Type, Addenda) => Unit)(using Context): Type =
13251339
var expected1 = alignDependentFunction(expected, actual.stripCapturing)
1340+
val falseDeps = expected1 ne expected
13261341
val actualBoxed = adapt(actual, expected1, tree)
13271342
//println(i"check conforms $actualBoxed <<< $expected1")
13281343

@@ -1332,26 +1347,23 @@ class CheckCaptures extends Recheck, SymTransformer:
13321347
TypeComparer.compareResult(isCompatible(actualBoxed, expected1)) match
13331348
case TypeComparer.CompareResult.Fail(notes) =>
13341349
capt.println(i"conforms failed for ${tree}: $actual vs $expected")
1335-
err.typeMismatch(tree.withType(actualBoxed), expected1,
1336-
addApproxAddenda(
1337-
addenda ++ errorNotes(notes),
1338-
expected1))
1350+
if falseDeps then expected1 = unalignFunction(expected1)
1351+
fail(tree.withType(actualBoxed), expected1,
1352+
addApproxAddenda(addenda ++ errorNotes(notes), expected1))
13391353
actual
13401354
case /*OK*/ _ =>
13411355
if debugSuccesses then tree match
1342-
case Ident(_) =>
1343-
println(i"SUCCESS $tree for $actual <:< $expected:\n${TypeComparer.explained(_.isSubType(actualBoxed, expected1))}")
1344-
case _ =>
1356+
case Ident(_) =>
1357+
println(i"SUCCESS $tree for $actual <:< $expected:\n${TypeComparer.explained(_.isSubType(actualBoxed, expected1))}")
1358+
case _ =>
13451359
actualBoxed
1346-
end checkConformsExpr
1360+
end testAdapted
13471361

13481362
/** Turn `expected` into a dependent function when `actual` is dependent. */
13491363
private def alignDependentFunction(expected: Type, actual: Type)(using Context): Type =
13501364
def recur(expected: Type): Type = expected.dealias match
1351-
case expected0 @ CapturingType(eparent, refs) =>
1352-
val eparent1 = recur(eparent)
1353-
if eparent1 eq eparent then expected
1354-
else CapturingType(eparent1, refs, boxed = expected0.isBoxed)
1365+
case expected @ CapturingType(eparent, refs) =>
1366+
expected.derivedCapturingType(recur(eparent), refs)
13551367
case expected @ defn.FunctionOf(args, resultType, isContextual)
13561368
if defn.isNonRefinedFunction(expected) =>
13571369
actual match
@@ -1369,6 +1381,14 @@ class CheckCaptures extends Recheck, SymTransformer:
13691381
case _ => expected
13701382
recur(expected)
13711383

1384+
private def unalignFunction(tp: Type)(using Context): Type = tp match
1385+
case tp @ CapturingType(parent, refs) =>
1386+
tp.derivedCapturingType(unalignFunction(parent), refs)
1387+
case defn.RefinedFunctionOf(mt) =>
1388+
mt.toFunctionType(alwaysDependent = false)
1389+
case _ =>
1390+
tp
1391+
13721392
/** For the expected type, implement the rule outlined in #14390:
13731393
* - when checking an expression `a: Ta^Ca` against an expected type `Te^Ce`,
13741394
* - where the capture set `Ce` contains Cls.this,

‎compiler/src/dotty/tools/dotc/cc/Setup.scala‎

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -406,19 +406,26 @@ class Setup extends PreRecheck, SymTransformer, SetupAPI:
406406
CapturingType(fntpe, cs, boxed = false)
407407
else fntpe
408408

409-
/** Check that types extending SharedCapability don't have a `cap` in their capture set.
410-
* TODO This is not enough.
411-
* We need to also track that we cannot get exclusive capabilities in paths
412-
* where some prefix derives from SharedCapability. Also, can we just
413-
* exclude `cap`, or do we have to extend this to all exclusive capabilties?
414-
* The problem is that we know what is exclusive in general only after capture
415-
* checking, not before.
409+
/** 1. Check that parents of capturing types are not pure.
410+
* 2. Check that types extending SharedCapability don't have a `cap` in their capture set.
411+
* TODO This is not enough.
412+
* We need to also track that we cannot get exclusive capabilities in paths
413+
* where some prefix derives from SharedCapability. Also, can we just
414+
* exclude `cap`, or do we have to extend this to all exclusive capabilties?
415+
* The problem is that we know what is exclusive in general only after capture
416+
* checking, not before.
416417
*/
417-
def checkSharedOK(tp: Type): tp.type =
418+
def checkRetainsOK(tp: Type): tp.type =
418419
tp match
419-
case CapturingType(parent, refs)
420-
if refs.isUniversal && parent.derivesFromSharedCapability =>
421-
fail(em"$tp extends SharedCapability, so it cannot capture `cap`")
420+
case CapturingType(parent, refs) =>
421+
if parent.isAlwaysPure && !tptToCheck.span.isZeroExtent then
422+
// If tptToCheck is zero-extent it could be copied from an overridden
423+
// method's result type. In that case, there's no point requiring
424+
// an explicit result type in the override, the inherited capture set
425+
// will be ignored anyway.
426+
fail(em"$parent is a pure type, it makes no sense to add a capture set to it")
427+
else if refs.isUniversal && parent.derivesFromSharedCapability then
428+
fail(em"$tp extends SharedCapability, so it cannot capture `cap`")
422429
case _ =>
423430
tp
424431

@@ -436,7 +443,7 @@ class Setup extends PreRecheck, SymTransformer, SetupAPI:
436443
def innerApply(t: Type) =
437444
t match
438445
case t @ CapturingType(parent, refs) =>
439-
checkSharedOK:
446+
checkRetainsOK:
440447
t.derivedCapturingType(stripImpliedCaptureSet(this(parent)), refs)
441448
case t @ AnnotatedType(parent, ann) =>
442449
val parent1 = this(parent)
@@ -445,7 +452,7 @@ class Setup extends PreRecheck, SymTransformer, SetupAPI:
445452
if !tptToCheck.isEmpty then
446453
checkWellformedLater(parent2, ann.tree, tptToCheck)
447454
try
448-
checkSharedOK:
455+
checkRetainsOK:
449456
CapturingType(parent2, ann.tree.toCaptureSet)
450457
catch case ex: IllegalCaptureRef =>
451458
if !tptToCheck.isEmpty then

‎compiler/src/dotty/tools/dotc/core/Types.scala‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1657,19 +1657,17 @@ object Types extends TypeUtils {
16571657
NoType
16581658
}
16591659

1660-
/** The iterator of underlying types as long as type is a TypeProxy.
1661-
* Useful for diagnostics
1660+
/** The iterator of underlying types staring with `this` and followed by
1661+
* repeatedly applying `f` as long as type is a TypeProxy. Useful for diagnostics.
16621662
*/
1663-
def underlyingIterator(using Context): Iterator[Type] = new Iterator[Type] {
1663+
def iterate(f: TypeProxy => Type): Iterator[Type] = new Iterator[Type]:
16641664
var current = Type.this
16651665
var hasNext = true
1666-
def next() = {
1666+
def next() =
16671667
val res = current
16681668
hasNext = current.isInstanceOf[TypeProxy]
1669-
if (hasNext) current = current.asInstanceOf[TypeProxy].underlying
1669+
if hasNext then current = f(current.asInstanceOf[TypeProxy])
16701670
res
1671-
}
1672-
}
16731671

16741672
/** A prefix-less refined this or a termRef to a new skolem symbol
16751673
* that has the given type as info.

‎compiler/src/dotty/tools/dotc/printing/PlainPrinter.scala‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -282,7 +282,7 @@ class PlainPrinter(_ctx: Context) extends Printer {
282282
(" <: " ~ toText(bound) provided !bound.isAny)
283283
}.close
284284
case tp @ CapturingType(parent, refs) =>
285-
val boxText: Text = Str("box ") provided tp.isBoxed //&& ctx.settings.YccDebug.value
285+
val boxText: Text = Str("box ") provided tp.isBoxed && ccVerbose
286286
if elideCapabilityCaps
287287
&& parent.derivesFrom(defn.Caps_Capability)
288288
&& refs.containsTerminalCapability

‎scala2-library-cc/src/scala/collection/Iterable.scala‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -994,7 +994,7 @@ trait SortedSetFactoryDefaults[+A,
994994
+WithFilterCC[x] <: IterableOps[x, WithFilterCC, WithFilterCC[x]] with Set[x]] extends SortedSetOps[A @uncheckedVariance, CC, CC[A @uncheckedVariance]] {
995995
self: IterableOps[A, WithFilterCC, _] =>
996996

997-
override protected def fromSpecific(coll: IterableOnce[A @uncheckedVariance]^): CC[A @uncheckedVariance]^{coll} = sortedIterableFactory.from(coll)(using ordering)
997+
override protected def fromSpecific(coll: IterableOnce[A @uncheckedVariance]^): CC[A @uncheckedVariance] = sortedIterableFactory.from(coll)(using ordering)
998998
override protected def newSpecificBuilder: mutable.Builder[A @uncheckedVariance, CC[A @uncheckedVariance]] = sortedIterableFactory.newBuilder[A](using ordering)
999999
override def empty: CC[A @uncheckedVariance] = sortedIterableFactory.empty(using ordering)
10001000

@@ -1050,7 +1050,7 @@ trait SortedMapFactoryDefaults[K, +V,
10501050
self: IterableOps[(K, V), WithFilterCC, _] =>
10511051

10521052
override def empty: CC[K, V @uncheckedVariance] = sortedMapFactory.empty(using ordering)
1053-
override protected def fromSpecific(coll: IterableOnce[(K, V @uncheckedVariance)]^): CC[K, V @uncheckedVariance]^{coll} = sortedMapFactory.from(coll)(using ordering)
1053+
override protected def fromSpecific(coll: IterableOnce[(K, V @uncheckedVariance)]^): CC[K, V @uncheckedVariance] = sortedMapFactory.from(coll)(using ordering)
10541054
override protected def newSpecificBuilder: mutable.Builder[(K, V @uncheckedVariance), CC[K, V @uncheckedVariance]] = sortedMapFactory.newBuilder[K, V](using ordering)
10551055

10561056
override def withFilter(p: ((K, V)) => Boolean): collection.SortedMapOps.WithFilter[K, V, WithFilterCC, UnsortedCC, CC]^{p} =

‎tests/neg-custom-args/captures/boundschecks3.check‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,31 +1,31 @@
11
-- [E057] Type Mismatch Error: tests/neg-custom-args/captures/boundschecks3.scala:9:11 ---------------------------------
22
9 | val foo: C[Tree^] = ??? // error
33
| ^
4-
| Type argument box test.Tree^ does not conform to upper bound test.Tree in inferred type test.C[box test.Tree^]
4+
| Type argument test.Tree^ does not conform to upper bound test.Tree in inferred type test.C[test.Tree^]
55
|
6-
| where: ^ refers to the universal root capability
6+
| where: ^ refers to the universal root capability
77
|
88
| longer explanation available when compiling with `-explain`
99
-- [E057] Type Mismatch Error: tests/neg-custom-args/captures/boundschecks3.scala:10:11 --------------------------------
1010
10 | type T = C[Tree^] // error
1111
| ^
12-
| Type argument box test.Tree^ does not conform to upper bound test.Tree in inferred type test.C[box test.Tree^]
12+
| Type argument test.Tree^ does not conform to upper bound test.Tree in inferred type test.C[test.Tree^]
1313
|
14-
| where: ^ refers to the universal root capability
14+
| where: ^ refers to the universal root capability
1515
|
1616
| longer explanation available when compiling with `-explain`
1717
-- [E057] Type Mismatch Error: tests/neg-custom-args/captures/boundschecks3.scala:11:11 --------------------------------
1818
11 | val bar: T -> T = ??? // error
1919
| ^
20-
|Type argument box test.Tree^ does not conform to upper bound test.Tree in subpart test.C[box test.Tree^] of inferred type test.C[box test.Tree^] -> test.C[box test.Tree^]
20+
|Type argument test.Tree^ does not conform to upper bound test.Tree in subpart test.C[test.Tree^] of inferred type test.C[test.Tree^] -> test.C[test.Tree^]
2121
|
2222
|where: ^ refers to the universal root capability
2323
|
2424
| longer explanation available when compiling with `-explain`
2525
-- [E057] Type Mismatch Error: tests/neg-custom-args/captures/boundschecks3.scala:12:11 --------------------------------
2626
12 | val baz: C[Tree^] -> Unit = ??? // error
2727
| ^
28-
|Type argument box test.Tree^ does not conform to upper bound test.Tree in subpart test.C[box test.Tree^] of inferred type test.C[box test.Tree^] -> Unit
28+
|Type argument test.Tree^ does not conform to upper bound test.Tree in subpart test.C[test.Tree^] of inferred type test.C[test.Tree^] -> Unit
2929
|
3030
|where: ^ refers to the universal root capability
3131
|

‎tests/neg-custom-args/captures/box-adapt-cases.check‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
-- [E007] Type Mismatch Error: tests/neg-custom-args/captures/box-adapt-cases.scala:8:10 -------------------------------
22
8 | x.value(cap => cap.use()) // error, was OK
33
| ^^^^^^^^^^^^^^^^
4-
| Found: (cap: box Cap^?) => Int
5-
| Required: (cap: box Cap^) =>² Int
4+
| Found: (cap: Cap^?) => Int
5+
| Required: Cap^ =>² Int
66
|
77
| where: => refers to the universal root capability
88
| =>² refers to a fresh root capability created in method test1
@@ -12,14 +12,14 @@
1212
-- [E007] Type Mismatch Error: tests/neg-custom-args/captures/box-adapt-cases.scala:15:10 ------------------------------
1313
15 | x.value(cap => cap.use()) // error
1414
| ^^^^^^^^^^^^^^^^
15-
| Found: (cap: box Cap^{io}) ->{io} Int
16-
| Required: (cap: box Cap^{io}) -> Int
15+
| Found: (cap: Cap^{io}) ->{io} Int
16+
| Required: Cap^{io} -> Int
1717
|
1818
| longer explanation available when compiling with `-explain`
1919
-- [E007] Type Mismatch Error: tests/neg-custom-args/captures/box-adapt-cases.scala:29:10 ------------------------------
2020
29 | x.value(cap => cap.use()) // error
2121
| ^^^^^^^^^^^^^^^^
22-
| Found: (cap: box Cap^?) ->{io, fs} Int
23-
| Required: (cap: box Cap^{io, fs}) ->{io} Int
22+
| Found: (cap: Cap^?) ->{io, fs} Int
23+
| Required: Cap^{io, fs} ->{io} Int
2424
|
2525
| longer explanation available when compiling with `-explain`

‎tests/neg-custom-args/captures/box-adapt-contra.check‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,18 @@
11
-- Error: tests/neg-custom-args/captures/box-adapt-contra.scala:9:52 ---------------------------------------------------
22
9 | val f: (Cap^{c} -> Unit) -> Unit = useCap[Cap^{c}](c) // error
33
| ^^^^^^^^^^^^^^^^^^
4-
| Cap^{c} -> Unit cannot be box-converted to box Cap^{c} ->{c} Unit
5-
| since the additional capture set {c} resulting from box conversion is not allowed in box Cap^{c} -> Unit
4+
| Cap^{c} -> Unit cannot be box-converted to Cap^{c} ->{c} Unit
5+
| since the additional capture set {c} resulting from box conversion is not allowed in Cap^{c} -> Unit
66
-- Error: tests/neg-custom-args/captures/box-adapt-contra.scala:13:57 --------------------------------------------------
77
13 | val f1: (Cap^{c} => Unit) ->{c} Unit = useCap1[Cap^{c}](c) // error, was ok when cap was a root
88
| ^^^^^^^^^^^^^^^^^^^
9-
| Cap^{c} => Unit cannot be box-converted to box Cap^{c} ->{cap, c} Unit
10-
| since the additional capture set {c} resulting from box conversion is not allowed in box Cap^{c} => Unit
9+
| Cap^{c} => Unit cannot be box-converted to Cap^{c} ->{cap, c} Unit
10+
| since the additional capture set {c} resulting from box conversion is not allowed in Cap^{c} => Unit
1111
|
12-
| where: => refers to the universal root capability
13-
| cap is the universal root capability
12+
| where: => refers to the universal root capability
13+
| cap is the universal root capability
1414
-- Error: tests/neg-custom-args/captures/box-adapt-contra.scala:19:54 --------------------------------------------------
1515
19 | val f3: (Cap^{c} -> Unit) => Unit = useCap3[Cap^{c}](c) // error
1616
| ^^^^^^^^^^^^^^^^^^^
17-
| Cap^{c} -> Unit cannot be box-converted to box Cap^{c} ->{d, c} Unit
18-
| since the additional capture set {c} resulting from box conversion is not allowed in box Cap^{c} ->{d} Unit
17+
| Cap^{c} -> Unit cannot be box-converted to Cap^{c} ->{d, c} Unit
18+
| since the additional capture set {c} resulting from box conversion is not allowed in Cap^{c} ->{d} Unit

0 commit comments

Comments
 (0)