Skip to content

Commit

Permalink
Improve "constructor proxy shadows outer" handling (#17154)
Browse files Browse the repository at this point in the history
  • Loading branch information
dwijnand authored Mar 28, 2023
2 parents 3948ecc + 80b165c commit 3a830c8
Show file tree
Hide file tree
Showing 8 changed files with 167 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ enum ErrorMessageID(val isActive: Boolean = true) extends java.lang.Enum[ErrorMe
case InlineGivenShouldNotBeFunctionID // errorNumber 174
case ValueDiscardingID // errorNumber 175
case UnusedNonUnitValueID // errorNumber 176
case ConstrProxyShadowsID // errorNumber 177

def errorNumber = ordinal - 1

Expand Down
31 changes: 31 additions & 0 deletions compiler/src/dotty/tools/dotc/reporting/messages.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1305,6 +1305,37 @@ extends SyntaxMsg(VarArgsParamMustComeLastID) {

import typer.Typer.BindingPrec

class ConstrProxyShadows(proxy: TermRef, shadowed: Type, shadowedIsApply: Boolean)(using Context)
extends ReferenceMsg(ConstrProxyShadowsID), NoDisambiguation:

def clsString(using Context) = proxy.symbol.companionClass.showLocated
def shadowedString(using Context) = shadowed.termSymbol.showLocated
def appClause = if shadowedIsApply then " the apply method of" else ""
def appSuffix = if shadowedIsApply then ".apply" else ""

def msg(using Context) =
i"""Reference to constructor proxy for $clsString
|shadows outer reference to $shadowedString
|
|The instance needs to be created with an explicit `new`."""

def explain(using Context) =
i"""There is an ambiguity in the meaning of the call
|
| ${proxy.symbol.name}(...)
|
|It could mean creating an instance of $clsString with
|
| new ${proxy.symbol.companionClass.name}(...)
|
|Or it could mean calling$appClause $shadowedString as in
|
| ${shadowed.termSymbol.name}$appSuffix(...)
|
|To disambiguate, use an explicit `new` if you mean the former,
|or use a full prefix for ${shadowed.termSymbol.name} if you mean the latter."""
end ConstrProxyShadows

class AmbiguousReference(name: Name, newPrec: BindingPrec, prevPrec: BindingPrec, prevCtx: Context)(using Context)
extends ReferenceMsg(AmbiguousReferenceID), NoDisambiguation {

Expand Down
44 changes: 31 additions & 13 deletions compiler/src/dotty/tools/dotc/typer/Typer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -544,22 +544,40 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
unimported = saved1
foundUnderScala2 = saved2

def checkNotShadowed(ownType: Type) = ownType match
case ownType: TermRef if ownType.symbol.is(ConstructorProxy) =>
val shadowed = findRef(name, pt, EmptyFlags, ConstructorProxy, tree.srcPos)
if shadowed.exists then
report.error(
em"""Reference to constructor proxy for ${ownType.symbol.companionClass.showLocated}
|shadows outer reference to ${shadowed.termSymbol.showLocated}""", tree.srcPos)
case _ =>
/** Normally, returns `ownType` except if `ownType` is a constructor proxy,
* and there is another shadowed type accessible with the same name that is not:
* - if the prototype is an application:
* - if the shadowed type has a method alternative or an apply method,
* issue an ambiguity error
* - otherwise again return `ownType`
* - if the prototype is not an application, return the shadowed type
*/
def checkNotShadowed(ownType: Type): Type =
ownType match
case ownType: TermRef if ownType.symbol.is(ConstructorProxy) =>
findRef(name, pt, EmptyFlags, ConstructorProxy, tree.srcPos) match
case shadowed: TermRef =>
pt match
case pt: FunOrPolyProto =>
def err(shadowedIsApply: Boolean) =
report.error(ConstrProxyShadows(ownType, shadowed, shadowedIsApply), tree.srcPos)
if shadowed.denot.hasAltWith(sd => sd.symbol.is(Method, butNot = Accessor)) then
err(shadowedIsApply = false)
else if shadowed.member(nme.apply).hasAltWith(_.symbol.is(Method, butNot = Accessor)) then
err(shadowedIsApply = true)
case _ =>
return shadowed
case shadowed =>
case _ =>
ownType

def setType(ownType: Type): Tree =
checkNotShadowed(ownType)
val tree1 = ownType match
case ownType: NamedType if !prefixIsElidable(ownType) =>
ref(ownType).withSpan(tree.span)
val checkedType = checkNotShadowed(ownType)
val tree1 = checkedType match
case checkedType: NamedType if !prefixIsElidable(checkedType) =>
ref(checkedType).withSpan(tree.span)
case _ =>
tree.withType(ownType)
tree.withType(checkedType)
val tree2 = toNotNullTermRef(tree1, pt)
checkLegalValue(tree2, pt)
tree2
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,12 @@ be selected with `apply` (or be applied to arguments, in which case the `apply`
inserted).

Constructor proxies are also not allowed to shadow normal definitions. That is,
if an identifier resolves to a constructor proxy, and the same identifier is also
defined or imported in some other scope, an ambiguity is reported.
an ambiguity is reported, if

- an identifier resolves to a constructor proxy,
- the same identifier is also defined or imported in some other scope,
- the other reference can be applied to a (possibly empty) parameter list. That
is, it refers either to a method or to a value containing an apply method as member.

## Motivation

Expand Down
75 changes: 75 additions & 0 deletions tests/neg-custom-args/explain/constructor-proxy-shadowing.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
-- [E177] Reference Error: tests/neg-custom-args/explain/constructor-proxy-shadowing.scala:10:12 -----------------------
10 | val x = A22("") // error: shadowing
| ^^^
| Reference to constructor proxy for class A22 in class A
| shadows outer reference to method A22 in object Test
|
| The instance needs to be created with an explicit `new`.
|--------------------------------------------------------------------------------------------------------------------
| Explanation (enabled by `-explain`)
|- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
| There is an ambiguity in the meaning of the call
|
| A22(...)
|
| It could mean creating an instance of class A22 in class A with
|
| new A22(...)
|
| Or it could mean calling method A22 in object Test as in
|
| A22(...)
|
| To disambiguate, use an explicit `new` if you mean the former,
| or use a full prefix for A22 if you mean the latter.
--------------------------------------------------------------------------------------------------------------------
-- [E177] Reference Error: tests/neg-custom-args/explain/constructor-proxy-shadowing.scala:11:12 -----------------------
11 | val y = A33("") // error: shadowing
| ^^^
| Reference to constructor proxy for class A33 in class A
| shadows outer reference to object A33 in object Test
|
| The instance needs to be created with an explicit `new`.
|--------------------------------------------------------------------------------------------------------------------
| Explanation (enabled by `-explain`)
|- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
| There is an ambiguity in the meaning of the call
|
| A33(...)
|
| It could mean creating an instance of class A33 in class A with
|
| new A33(...)
|
| Or it could mean calling the apply method of object A33 in object Test as in
|
| A33.apply(...)
|
| To disambiguate, use an explicit `new` if you mean the former,
| or use a full prefix for A33 if you mean the latter.
--------------------------------------------------------------------------------------------------------------------
-- [E177] Reference Error: tests/neg-custom-args/explain/constructor-proxy-shadowing.scala:16:8 ------------------------
16 |val x = Seq(3) // error: shadowing
| ^^^
| Reference to constructor proxy for class Seq
| shadows outer reference to getter Seq in package scala
|
| The instance needs to be created with an explicit `new`.
|--------------------------------------------------------------------------------------------------------------------
| Explanation (enabled by `-explain`)
|- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
| There is an ambiguity in the meaning of the call
|
| Seq(...)
|
| It could mean creating an instance of class Seq with
|
| new Seq(...)
|
| Or it could mean calling the apply method of getter Seq in package scala as in
|
| Seq.apply(...)
|
| To disambiguate, use an explicit `new` if you mean the former,
| or use a full prefix for Seq if you mean the latter.
--------------------------------------------------------------------------------------------------------------------
16 changes: 16 additions & 0 deletions tests/neg-custom-args/explain/constructor-proxy-shadowing.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@

object Test extends App {
def A22(s: String): String = s
class A33(s: String)
object A33:
def apply(s: String) = ???
class A {
class A22(s: String)
class A33(s: String)
val x = A22("") // error: shadowing
val y = A33("") // error: shadowing
}
}

class Seq(n: Int)
val x = Seq(3) // error: shadowing
10 changes: 0 additions & 10 deletions tests/neg/constructor-proxy-shadowing.scala

This file was deleted.

7 changes: 7 additions & 0 deletions tests/pos/constr-proxy-shadowing.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class Number(n: Int)
val x = Number(3)

class Seq(n: Int)
val y = Seq


0 comments on commit 3a830c8

Please sign in to comment.