Skip to content

warn about unnecessary uses of .nn #23327

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
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
11 changes: 11 additions & 0 deletions compiler/src/dotty/tools/dotc/core/NullOpsDecorator.scala
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,17 @@ object NullOpsDecorator:
val stripped = self.stripNull()
stripped ne self
}

def admitsNull(using Context): Boolean = {
Copy link
Member

Choose a reason for hiding this comment

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

Since we are basically checking if a type has Null as a subtype, I'm not sure whether we should widen a singleton type.

For example,

val s: String | Null = ???
val ss: s.type | Null = ???
val snotnull: s.type = ss.nn

I'd like to allow ss.nn to say if it returns a value, the value is equal to s and not null.

self.isNullType || self.isAny || (self match
case OrType(l, r) => r.admitsNull || l.admitsNull
case AndType(l, r) => r.admitsNull && l.admitsNull
case TypeBounds(lo, hi) => lo.admitsNull
case FlexibleType(lo, hi) => true
case tp: TypeProxy => tp.underlying.admitsNull
case _ => false
)
}
end extension

import ast.tpd.*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ object OrderingConstraint {
}
else {
val prev_es = entries(prev, poly)
if (prev_es == null || (es.nn ne prev_es.nn))
if (prev_es == null || (es.nn ne prev_es))
current // can re-use existing entries array.
else {
es = es.nn.clone
Expand Down
1 change: 1 addition & 0 deletions compiler/src/dotty/tools/dotc/core/StdNames.scala
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,7 @@ object StdNames {
val next: N = "next"
val nmeNewTermName: N = "newTermName"
val nmeNewTypeName: N = "newTypeName"
val nn: N = "nn"
val noAutoTupling: N = "noAutoTupling"
val normalize: N = "normalize"
val notifyAll_ : N = "notifyAll"
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/core/SymDenotations.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2581,7 +2581,7 @@ object SymDenotations {
try f.container == chosen.container catch case NonFatal(ex) => true
if !ambiguityWarningIssued then
for conflicting <- assocFiles.find(!sameContainer(_)) do
report.warning(em"""${ambiguousFilesMsg(conflicting.nn)}
report.warning(em"""${ambiguousFilesMsg(conflicting)}
|Keeping only the definition in $chosen""")
ambiguityWarningIssued = true
multi.filterWithPredicate(_.symbol.associatedFile == chosen)
Expand Down
4 changes: 2 additions & 2 deletions compiler/src/dotty/tools/dotc/core/Types.scala
Original file line number Diff line number Diff line change
Expand Up @@ -373,9 +373,9 @@ object Types extends TypeUtils {
final def isNotNull(using Context): Boolean = this match {
case tp: ConstantType => tp.value.value != null
case tp: FlexibleType => false
case tp: ClassInfo => !tp.cls.isNullableClass && tp.cls != defn.NothingClass
case tp: ClassInfo => !tp.cls.isNullableClass && !tp.isNothingType
case tp: AppliedType => tp.superType.isNotNull
case tp: TypeBounds => tp.lo.isNotNull
case tp: TypeBounds => tp.hi.isNotNull
case tp: TypeProxy => tp.underlying.isNotNull
case AndType(tp1, tp2) => tp1.isNotNull || tp2.isNotNull
case OrType(tp1, tp2) => tp1.isNotNull && tp2.isNotNull
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/printing/Formatting.scala
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ object Formatting {

class ShowImplicits4:
given [X: Show]: Show[X | Null] with
def show(x: X | Null) = if x == null then "null" else CtxShow(toStr(x.nn))
def show(x: X | Null) = if x == null then "null" else CtxShow(toStr(x))

class ShowImplicits3 extends ShowImplicits4:
given Show[Product] = ShowAny
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/transform/Erasure.scala
Original file line number Diff line number Diff line change
Expand Up @@ -695,7 +695,7 @@ object Erasure {
val owner = sym.maybeOwner
if defn.specialErasure.contains(owner) then
assert(sym.isConstructor, s"${sym.showLocated}")
defn.specialErasure(owner).nn
defn.specialErasure(owner)
else if defn.isSyntheticFunctionClass(owner) then
defn.functionTypeErasure(owner).typeSymbol
else
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ object GenericSignatures {
typeParamSig(sym.name.lastPart)
}
else if (defn.specialErasure.contains(sym))
jsig(defn.specialErasure(sym).nn.typeRef)
jsig(defn.specialErasure(sym).typeRef)
else if (sym == defn.UnitClass || sym == defn.BoxedUnitModule)
jsig(defn.BoxedUnitClass.typeRef)
else if (sym == defn.NothingClass)
Expand Down
16 changes: 8 additions & 8 deletions compiler/src/dotty/tools/dotc/transform/LazyVals.scala
Original file line number Diff line number Diff line change
Expand Up @@ -477,12 +477,12 @@ class LazyVals extends MiniPhase with IdentityDenotTransformer {
newSymbol(claz, offsetName(info.defs.size), Synthetic, defn.LongType).enteredAfter(this)
case None =>
newSymbol(claz, offsetName(0), Synthetic, defn.LongType).enteredAfter(this)
offsetSymbol.nn.addAnnotation(Annotation(defn.ScalaStaticAnnot, offsetSymbol.nn.span))
offsetSymbol.addAnnotation(Annotation(defn.ScalaStaticAnnot, offsetSymbol.span))
val fieldTree = thizClass.select(lazyNme.RLazyVals.getDeclaredField).appliedTo(Literal(Constant(containerName.mangledString)))
val offsetTree = ValDef(offsetSymbol.nn, getOffset.appliedTo(fieldTree))
val offsetTree = ValDef(offsetSymbol, getOffset.appliedTo(fieldTree))
val offsetInfo = appendOffsetDefs.getOrElseUpdate(claz, new OffsetInfo(Nil))
offsetInfo.defs = offsetTree :: offsetInfo.defs
val offset = ref(offsetSymbol.nn)
val offset = ref(offsetSymbol)

val swapOver =
This(claz)
Expand Down Expand Up @@ -617,31 +617,31 @@ class LazyVals extends MiniPhase with IdentityDenotTransformer {
.symbol.asTerm
else { // need to create a new flag
offsetSymbol = newSymbol(claz, offsetById, Synthetic, defn.LongType).enteredAfter(this)
offsetSymbol.nn.addAnnotation(Annotation(defn.ScalaStaticAnnot, offsetSymbol.nn.span))
offsetSymbol.addAnnotation(Annotation(defn.ScalaStaticAnnot, offsetSymbol.span))
val flagName = LazyBitMapName.fresh(id.toString.toTermName)
val flagSymbol = newSymbol(claz, flagName, containerFlags, defn.LongType).enteredAfter(this)
flag = ValDef(flagSymbol, Literal(Constant(0L)))
val fieldTree = thizClass.select(lazyNme.RLazyVals.getDeclaredField).appliedTo(Literal(Constant(flagName.toString)))
val offsetTree = ValDef(offsetSymbol.nn, getOffsetStatic.appliedTo(fieldTree))
val offsetTree = ValDef(offsetSymbol, getOffsetStatic.appliedTo(fieldTree))
info.defs = offsetTree :: info.defs
}

case None =>
offsetSymbol = newSymbol(claz, offsetName(0), Synthetic, defn.LongType).enteredAfter(this)
offsetSymbol.nn.addAnnotation(Annotation(defn.ScalaStaticAnnot, offsetSymbol.nn.span))
offsetSymbol.addAnnotation(Annotation(defn.ScalaStaticAnnot, offsetSymbol.span))
val flagName = LazyBitMapName.fresh("0".toTermName)
val flagSymbol = newSymbol(claz, flagName, containerFlags, defn.LongType).enteredAfter(this)
flag = ValDef(flagSymbol, Literal(Constant(0L)))
val fieldTree = thizClass.select(lazyNme.RLazyVals.getDeclaredField).appliedTo(Literal(Constant(flagName.toString)))
val offsetTree = ValDef(offsetSymbol.nn, getOffsetStatic.appliedTo(fieldTree))
val offsetTree = ValDef(offsetSymbol, getOffsetStatic.appliedTo(fieldTree))
appendOffsetDefs += (claz -> new OffsetInfo(List(offsetTree), ord))
}

val containerName = LazyLocalName.fresh(x.name.asTermName)
val containerSymbol = newSymbol(claz, containerName, x.symbol.flags &~ containerFlagsMask | containerFlags, tpe, coord = x.symbol.coord).enteredAfter(this)
val containerTree = ValDef(containerSymbol, defaultValue(tpe))

val offset = ref(offsetSymbol.nn)
val offset = ref(offsetSymbol)
val getFlag = Select(ref(defn.LazyValsModule), lazyNme.RLazyVals.get)
val setFlag = Select(ref(defn.LazyValsModule), lazyNme.RLazyVals.setFlag)
val wait = Select(ref(defn.LazyValsModule), lazyNme.RLazyVals.wait4Notification)
Expand Down
6 changes: 3 additions & 3 deletions compiler/src/dotty/tools/dotc/typer/ProtoTypes.scala
Original file line number Diff line number Diff line change
Expand Up @@ -466,13 +466,13 @@ object ProtoTypes {
targ = typerFn(arg)
// TODO: investigate why flow typing is not working on `targ`
if ctx.reporter.hasUnreportedErrors then
if hasInnerErrors(targ.nn, argType) then
if hasInnerErrors(targ, argType) then
state.errorArgs += arg
else
state.typedArg = state.typedArg.updated(arg, targ.nn)
state.typedArg = state.typedArg.updated(arg, targ)
state.errorArgs -= arg
}
targ.nn
targ
}

/** The typed arguments. This takes any arguments already typed using
Expand Down
17 changes: 16 additions & 1 deletion compiler/src/dotty/tools/dotc/typer/Typer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1088,7 +1088,19 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
errorTree(tree, em"cannot convert to type selection") // will never be printed due to fallback
}

if (tree.qualifier.isType) {
def warnUnnecessaryNN(tree: Tree): Unit = {
if ctx.explicitNulls then {
Copy link
Member

Choose a reason for hiding this comment

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

Do you agree we should check unnecessary nn even without explicit nulls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, but I didn't want to rock the boat too much before we fine-tune which corner cases should/shouldn't warn.

val symbol = tree.symbol
if symbol.exists && symbol.owner == defn.ScalaPredefModuleClass && symbol.name == nme.nn then
tree match
case Apply(_, args) =>
if(args.head.tpe.isNotNull) then report.warning("Unnecessary .nn: qualifier is already not null", tree)
if pt.admitsNull then report.warning("Unnecessary .nn: expected type admits null", tree)
case _ =>
}
}

val tree1 = if (tree.qualifier.isType) {
val qual1 = typedType(tree.qualifier, shallowSelectionProto(tree.name, pt, this, tree.nameSpan))
assignType(cpy.Select(tree)(qual1, tree.name), qual1)
}
Expand All @@ -1098,6 +1110,9 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
tryAlternatively(typeSelectOnTerm)(tryJavaSelectOnType)
else
typeSelectOnTerm

warnUnnecessaryNN(tree1)
tree1
}

def typedThis(tree: untpd.This)(using Context): Tree = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ class CompilerSearchVisitor(
)(using ctx: Context, reports: ReportContext)
extends SymbolSearchVisitor:

val logger: Logger = Logger.getLogger(classOf[CompilerSearchVisitor].getName().nn).nn
val logger: Logger = Logger.getLogger(classOf[CompilerSearchVisitor].getName()).nn

private def isAccessibleImplicitClass(sym: Symbol) =
val owner = sym.maybeOwner
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ object SignatureHelpProvider:
case Some(paramDoc) =>
val newName =
if isJavaSymbol && head.name.startsWith("x$") then
paramDoc.nn.displayName()
paramDoc.displayName()
else head.name
head.copy(name = newName.nn, doc = Some(paramDoc.docstring.nn)) :: rest
case _ => head :: rest
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ object CompletionPos:
CompletionPos(
start,
identEnd,
query.nn,
query,
sourcePos,
offsetParams.uri.nn,
wasCursorApplied,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ class CompletionProvider(
*/
private def applyCompletionCursor(params: OffsetParams): (Boolean, String) =
val text = params.text().nn
val offset = params.offset().nn
val offset = params.offset()
val query = Completion.naiveCompletionPrefix(text, offset)

if offset > 0 && text.charAt(offset - 1).isUnicodeIdentifierPart
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ object TestInlayHints {
InlayHints.fromData(inlayHint.getData().asInstanceOf[JsonElement])._2
buffer += "/*"
labels.zip(data).foreach { case (label, data) =>
buffer += label.nn
buffer += label
buffer ++= readData(data)
}
buffer += "*/"
Expand Down
18 changes: 18 additions & 0 deletions tests/explicit-nulls/warn/unnecessary-nn.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
def f1(s: String): String = s.nn // warn
def f2(s: String|Null): String|Null = s.nn // warn
def f3(s: String|Null): Any = s.nn // warn
def f4(s: String|Null): String = s.nn

def f5[T >: String](s: String|Null): T = s.nn
def f6[T >: String|Null](s: String|Null): T = s.nn // warn

def f5a[T <: String](s: T): String = s.nn // warn

// flexible types
def f7(s: String|Null) = "".concat(s.nn) // warn
def f8(s: String): String = s.trim().nn // OK because the .nn could be useful as a dynamic null check


def f9(s: String|Null): String =
if(s == null) "default"
else s.nn // warn
Loading