Skip to content

Warn users on non canonical type references #9952

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 20, 2020
Merged

Warn users on non canonical type references #9952

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion community-build/community-projects/shapeless
Open in desktop
Submodule shapeless updated 1 files
+1 −1 modules/deriving/src/main/scala/shapeless3/deriving/annotation.scala
7 changes: 5 additions & 2 deletions compiler/src/dotty/tools/dotc/typer/QuotesAndSplices.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ trait QuotesAndSplices {
val tree1 =
if ctx.mode.is(Mode.Pattern) then
typedQuotePattern(tree, pt, qctx)
else if (tree.quoted.isType)
else if tree.quoted.isType then
report.warning(em"Consider using canonical type constructor scala.quoted.Type[${tree.quoted}] instead", tree.srcPos)
typedTypeApply(untpd.TypeApply(untpd.ref(defn.QuotedTypeModule_apply.termRef), tree.quoted :: Nil), pt)(using quoteContext).select(nme.apply).appliedTo(qctx)
else
typedApply(untpd.Apply(untpd.ref(defn.InternalQuoted_exprQuote.termRef), tree.quoted), pt)(using pushQuoteContext(qctx)).select(nme.apply).appliedTo(qctx)
Expand Down Expand Up @@ -171,7 +172,9 @@ trait QuotesAndSplices {
using spliceContext.retractMode(Mode.QuotedPattern).withOwner(spliceOwner(ctx)))
pat.select(tpnme.spliceType)
else
typedSelect(untpd.Select(tree.expr, tpnme.spliceType), pt)(using spliceContext).withSpan(tree.span)
val tree1 = typedSelect(untpd.Select(tree.expr, tpnme.spliceType), pt)(using spliceContext).withSpan(tree.span)
report.warning(em"Consider using canonical type reference ${tree1.tpe.show} instead", tree.srcPos)
tree1
}

private def checkSpliceOutsideQuote(tree: untpd.Tree)(using Context): Unit =
Expand Down
2 changes: 1 addition & 1 deletion tests/patmat/i9489.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import scala.quoted._

def summonTypedType[T : Type](using QuoteContext): String = '[T] match {
def summonTypedType[T : Type](using QuoteContext): String = Type[T] match {
case '[Boolean] => "Boolean"
case '[Byte] => "Byte"
case _ => "Other"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems '[T] is more symmetric here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, and that is why it should not be used. The syntactic symmetry is what lead to most conceptual misunderstandings of how Type works.

This constructor should also never be used explicitly and giving it a shorter alias does not help users avoid the explicit use.

It is also uncertain that the case '[...] pattern will be kept. We might remove them and use only a single matching abstraction as we do with inline match.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the usage is bad, what about disallowing it in syntax?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is my intent as I explain in #9486. This is a first step to help migration. We can drop the syntax later on once users have adapted. I would prefer to have at least one release with a warning.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to bring this up in the meeting, as it's syntax related.

Copy link
Contributor Author

@nicolasstucki nicolasstucki Oct 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This specific case is a corner case that should be avoided. It should have been written explicitly as

def summonTypedType[T : Type](using QuoteContext): String = summon[Type[T]] match { ...
// or 
def summonTypedType[T](using tp: Type[T])(using QuoteContext): String = tp match { ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rewrite this test to remove the '[Byte] syntax?

Expand Down
7 changes: 7 additions & 0 deletions tests/patmat/i9489b.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import scala.quoted._

def summonTypedType[T : Type](using QuoteContext): String = '{ ??? : T } match {
case '{ $_ : Boolean } => "Boolean"
case '{ $_ : Byte } => "Byte"
case _ => "Other"
}