-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add quick fix to add .nn #23598
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
base: main
Are you sure you want to change the base?
Add quick fix to add .nn #23598
Conversation
cc @noti0na1 |
override def actions(using Context) = | ||
if shouldSuggestNN then | ||
inTree match { | ||
case Some(tree) if tree != null => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you encountered a null
tree during testing?
@@ -300,6 +300,11 @@ extends NotFoundMsg(MissingIdentID) { | |||
class TypeMismatch(val found: Type, expected: Type, val inTree: Option[untpd.Tree], addenda: => String*)(using Context) | |||
extends TypeMismatchMsg(found, expected)(TypeMismatchID): | |||
|
|||
private val shouldSuggestNN = | |||
if expected.isValueType then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a condition ctx.mode.is(SafeNulls)
so we only compute the subtyping in explicit nulls
case _ => | ||
List() | ||
} | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can get rid of the outter if
by moving shouldSuggestNN
to the match case condition.
List( | ||
CodeAction(title = """Add .nn""", | ||
description = None, | ||
patches = List( | ||
ActionPatch(tree.srcPos.sourcePos, replacement) | ||
) | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's format the indent a little nicer
case Some(tree) if tree != null => | ||
val content = tree.source.content().slice(tree.srcPos.startPos.start, tree.srcPos.endPos.end).mkString | ||
val replacement = tree match | ||
case Apply(fun, args) => "(" + content + ").nn" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safer to add "( )" by default? For example, (if ... then ... else ...).nn
. Then, we only add "( )" if the tree is a ident, select, or a regular apply?
@@ -179,6 +179,79 @@ class CodeActionTest extends DottyTest: | |||
ctxx = ctxx | |||
) | |||
|
|||
@Test def addNN = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also test function applications where the argument need .nn
and some complex expressions, like a match or if.
An extension to #23461 that adds a quick fix to add a .nn. For example, if the code were
the quick fix would transform the code to
The code is WIP (I've put some ugly workarounds to get it to work). Right now, it adds brackets whenever the root node is an Apply. This is needed for infix functions (both those with ApplyKind InfixTuple and those that are written as such in the source code, such as + which is desugared to an ApplyKind Regular I think), but there may be other edge cases where brackets are needed. I've discussed these edges cases a bit with @olhotak and @HarrisL2, but it's possible that we've missed some (or I've forgotten to add them).