Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve "constructor proxy shadows outer" handling #17154

Merged
merged 2 commits into from
Mar 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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