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

Add IgnoreUnusedBindingExpression quick-fix #164

Draft
wants to merge 17 commits into
base: net203
Choose a base branch
from
Draft
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 @@ -159,6 +159,7 @@
<Compile Include="src\QuickFixes\AddExtensionAttributeFix.fs" />
<Compile Include="src\QuickFixes\RemoveRedundantParenExprFix.fs" />
<Compile Include="src\QuickFixes\NamespaceToModuleFix.fs" />
<Compile Include="src\QuickFixes\IgnoreUnusedBindingExpressionFix.fs" />
<Compile Include="src\PostfixTemplates\PostfixTemplates.fs" />
<Compile Include="src\PostfixTemplates\NotTemplate.fs" />
<Compile Include="src\PostfixTemplates\LetPostfixTemplate.fs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
<QuickFix>AddUnderscorePrefixFix</QuickFix>
<QuickFix>RemoveUnusedLocalBindingFix</QuickFix>
<QuickFix>RemoveUnusedNamedAsPatFix</QuickFix>
<QuickFix>IgnoreUnusedBindingExpressionFix</QuickFix>
</Warning>

<Warning staticGroup="FSharpErrors" name="UnitTypeExpected">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,5 @@ type AddIgnoreFix(expr: IFSharpExpression) =
override x.ExecutePsiTransaction _ =
use writeCookie = WriteLockCookie.Create(expr.IsPhysical())
use disableFormatter = new DisableCodeFormatter()

let ignoreApp = expr.CreateElementFactory().CreateIgnoreApp(expr, shouldAddNewLine expr)

let replaced = ModificationUtil.ReplaceChild(expr, ignoreApp).As<IBinaryAppExpr>()
addParensIfNeeded replaced.LeftArgument |> ignore
ignoreExpression expr (shouldAddNewLine expr)
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
namespace JetBrains.ReSharper.Plugins.FSharp.Psi.Features.Daemon.QuickFixes

open JetBrains.ReSharper.Plugins.FSharp.Psi.Features.Daemon.Highlightings
open JetBrains.ReSharper.Plugins.FSharp.Psi.Impl
open JetBrains.ReSharper.Plugins.FSharp.Psi.Impl.Tree
open JetBrains.ReSharper.Plugins.FSharp.Psi.Tree
open JetBrains.ReSharper.Plugins.FSharp.Psi
open JetBrains.ReSharper.Plugins.FSharp.Psi.Features.Util
open JetBrains.ReSharper.Psi
open JetBrains.ReSharper.Psi.ExtensionsAPI.Tree
open JetBrains.ReSharper.Psi.Tree
open JetBrains.ReSharper.Resources.Shell

type IgnoreUnusedBindingExpressionFix(warning: UnusedValueWarning) =
inherit FSharpQuickFixBase()

let pat = warning.Pat.IgnoreParentParens()
let binding = BindingNavigator.GetByHeadPattern(pat)
let letOrUseExpr = LetOrUseExprNavigator.GetByBinding(binding)

let rec getCorrectAnchor (expr: ITreeNode): ITreeNode =
match expr with
| :? ISequentialExpr as seqExpr when not (seqExpr.ExpressionsEnumerable.IsEmpty()) ->
let last = seqExpr.ExpressionsEnumerable.LastOrDefault()
if last :? ILetOrUseExpr then getCorrectAnchor last else last :> _

| :? ILetOrUseExpr as letExpr when isNotNull letExpr.InExpression -> getCorrectAnchor letExpr.InExpression
| _ ->

let exprCopy = expr.Copy()
let seqExpr = ModificationUtil.ReplaceChild(expr, ElementType.SEQUENTIAL_EXPR.Create())
ModificationUtil.AddChild(seqExpr, exprCopy)

override x.Text = "Ignore expression"

override x.IsAvailable _ =
isValid pat && not (pat :? IParametersOwnerPat) && isValid letOrUseExpr && letOrUseExpr.Bindings.Count = 1 &&
isValid binding.Expression && not (binding.Expression :? IDoExpr)

override x.ExecutePsiTransaction _ =
use writeLock = WriteLockCookie.Create(pat.IsPhysical())
use formatter = FSharpRegistryUtil.AllowFormatterCookie.Create()

if not (binding.Expression.Type().IsVoid()) then
ignoreInnermostExpression binding.Expression false

let inExpr = letOrUseExpr.InExpression
let newLine = NewLine(letOrUseExpr.GetLineEnding())

let bindingExpr = ModificationUtil.ReplaceChild(letOrUseExpr, binding.Expression)
addNodesAfter (getCorrectAnchor bindingExpr) [
newLine
newLine
inExpr
] |> ignore
Original file line number Diff line number Diff line change
Expand Up @@ -108,3 +108,22 @@ let setBindingExpression (expr: IFSharpExpression) contextIndent (letBindings: #
ModificationUtil.AddChildBefore(newExpr, NewLine(expr.GetLineEnding())) |> ignore
ModificationUtil.AddChildBefore(newExpr, Whitespace(contextIndent + indentSize)) |> ignore
shiftExpr indentSize newExpr

let ignoreExpression (expr: IFSharpExpression) shouldAddNewLine =
let ignoreApp = expr.CreateElementFactory().CreateIgnoreApp(expr, shouldAddNewLine)

let replaced = ModificationUtil.ReplaceChild(expr, ignoreApp).As<IBinaryAppExpr>()
addParensIfNeeded replaced.LeftArgument |> ignore

let ignoreInnermostExpression (expr: IFSharpExpression) shouldAddNewLine =
let rec getInnermostExpression (expr: IFSharpExpression) =
match expr with
| :? ISequentialExpr as seqExpr when not (seqExpr.ExpressionsEnumerable.IsEmpty()) ->
getInnermostExpression (seqExpr.ExpressionsEnumerable.LastOrDefault())
| :? ILetOrUseExpr as letOrUseExpr when isNotNull letOrUseExpr.InExpression ->
getInnermostExpression letOrUseExpr.InExpression
| :? IParenExpr as parenExpr when isNotNull parenExpr.InnerExpression ->
getInnermostExpression parenExpr.InnerExpression
| _ -> expr

ignoreExpression (getInnermostExpression expr) shouldAddNewLine
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,22 @@ private void Aligning()

private void Formatting()
{
Describe<FormattingRule>()
.Name("LineBreakBeforeIgnorePipe")
.Group(LineBreaksRuleGroup)
.Where(
Parent()
.HasType(ElementType.BINARY_APP_EXPR)
.Satisfies((node, context) => ((IBinaryAppExpr) node).RightArgument.GetText() == "ignore"),
Left()
.HasRole(BinaryAppExpr.LEFT_ARG_EXPR)
.Satisfies((node, context) => node.ContainsLineBreak(context.CodeFormatter))
.In(ElementType.MATCH_EXPR, ElementType.IF_THEN_ELSE_EXPR, ElementType.TRY_WITH_EXPR,
ElementType.TRY_FINALLY_EXPR, ElementType.LAMBDA_EXPR, ElementType.ASSERT_EXPR,
ElementType.LAZY_EXPR, ElementType.MATCH_LAMBDA_EXPR))
.Return(IntervalFormatType.NewLine)
.Build();

Describe<FormattingRule>()
.Group(SpaceRuleGroup)
.Name("SpaceAfterImplicitConstructorDecl")
Expand Down Expand Up @@ -285,7 +301,8 @@ private void DescribeSimpleAlignmentRule((string name, CompositeNodeType nodeTyp
}

private static bool IndentElseExpr(ITreeNode elseExpr, CodeFormattingContext context) =>
elseExpr.GetPreviousMeaningfulSibling().IsFirstOnLine(context.CodeFormatter) && !(elseExpr is IElifExpr);
(elseExpr.GetPreviousMeaningfulSibling().IsFirstOnLine(context.CodeFormatter) ||
!AreAligned(elseExpr, elseExpr.Parent?.FirstChild, context.CodeFormatter)) && !(elseExpr is IElifExpr);

private static bool AreAligned(ITreeNode first, ITreeNode second, IWhitespaceChecker whitespaceChecker) =>
first.CalcLineIndent(whitespaceChecker) == second.CalcLineIndent(whitespaceChecker);
Expand Down
2 changes: 1 addition & 1 deletion ReSharper.FSharp/src/FSharp.Psi/src/FSharp.psi
Original file line number Diff line number Diff line change
Expand Up @@ -653,7 +653,7 @@ doExpr options { stubBase="UnitExpressionBase"; }:
DO<KEYWORD, Keyword>
fSharpExpression<EXPR, Expression>;

assertExpr options { stubBase="FSharpExpressionBase"; }:
assertExpr options { stubBase="UnitExpressionBase"; }:
ASSERT<KEYWORD, Keyword>
fSharpExpression<EXPR, Expression>;

Expand Down
3 changes: 3 additions & 0 deletions ReSharper.FSharp/src/FSharp.Psi/src/Impl/Tree/LetOrUseExpr.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using JetBrains.Diagnostics;
using JetBrains.ReSharper.Plugins.FSharp.Psi.Parsing;
using JetBrains.ReSharper.Psi;

namespace JetBrains.ReSharper.Plugins.FSharp.Psi.Impl.Tree
{
Expand All @@ -18,5 +19,7 @@ public void SetIsRecursive(bool value)

LetOrUseToken.NotNull().AddTokenAfter(FSharpTokenType.REC);
}

public override IType Type() => InExpression.Type();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
using JetBrains.ReSharper.Psi;

namespace JetBrains.ReSharper.Plugins.FSharp.Psi.Impl.Tree
{
internal partial class TryFinallyExpr
{
public override IType Type() => TryExpression.Type();
}
}
9 changes: 9 additions & 0 deletions ReSharper.FSharp/src/FSharp.Psi/src/Impl/Tree/TryWithExpr.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
using JetBrains.ReSharper.Psi;

namespace JetBrains.ReSharper.Plugins.FSharp.Psi.Impl.Tree
{
internal partial class TryWithExpr
{
public override IType Type() => TryExpression.Type();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ Replace with '_'
--Replace unused values with '_' in solution
Rename to '_foo'
Remove unused value
Ignore expression
1: The value 'foo bar' is unused
QUICKFIXES:
Replace with '_'
Expand All @@ -23,3 +24,4 @@ Replace with '_'
--Replace unused values with '_' in project
--Replace unused values with '_' in solution
Remove unused value
Ignore expression
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
module Module

let a =
Copy link
Collaborator

@auduchinok auduchinok Aug 14, 2020

Choose a reason for hiding this comment

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

I think we could save up to a half of the dump by using do statement instead of the top-level binding.

module Module

do
  let a =
    1
    1

  ()

A bonus point would be replacing the top-level module with a anon module, but it'd require more changes in test project options, so I'd rather do it separately.

let b{caret} =
1
1

()
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
IFSharpImplFile
INamedModuleDeclaration
FSharpTokenType+ModuleTokenElement(type:MODULE, text:module)
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
FSharpIdentifierToken(type:IDENTIFIER, text:Module)
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
ILetModuleDecl
FSharpTokenType+LetTokenElement(type:LET, text:let)
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
ITopBinding
ITopReferencePat
IExpressionReferenceName
FSharpIdentifierToken(type:IDENTIFIER, text:a)
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
FSharpTokenType+EqualsTokenElement(type:EQUALS, text:=)
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
IChameleonExpression
ISequentialExpr
ILiteralExpr
FSharpToken(type:INT32, text:1)
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
IBinaryAppExpr
ILiteralExpr
FSharpToken(type:INT32, text:1)
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
IReferenceExpr
FSharpIdentifierToken(type:SYMBOLIC_OP, text:|>)
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
IReferenceExpr
FSharpIdentifierToken(type:IDENTIFIER, text:ignore)
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
IUnitExpr
FSharpTokenType+LparenTokenElement(type:LPAREN, text:()
FSharpTokenType+RparenTokenElement(type:RPAREN, text:))
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
module Module

let a =
1{caret}
1 |> ignore

()
Copy link
Collaborator

@auduchinok auduchinok Aug 11, 2020

Choose a reason for hiding this comment

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

Please make sure it's a correct sequence expression node after the modification, i.e. it should be something like

seqExpr
  1
  1 |> ignore
  ()

and not like

seqExpr
  seqExpr
    1
    1 |> ignore
  ()

It'd be great to have this logic reused in InlineVar refactoring later.

We should probably dump the resulting tree in some of the tests with modifications.

Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
module Module

let a =
let b{caret} =
let c = 1
c + c

()
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
IFSharpImplFile
INamedModuleDeclaration
FSharpTokenType+ModuleTokenElement(type:MODULE, text:module)
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
FSharpIdentifierToken(type:IDENTIFIER, text:Module)
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
ILetModuleDecl
FSharpTokenType+LetTokenElement(type:LET, text:let)
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
ITopBinding
ITopReferencePat
IExpressionReferenceName
FSharpIdentifierToken(type:IDENTIFIER, text:a)
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
FSharpTokenType+EqualsTokenElement(type:EQUALS, text:=)
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
IChameleonExpression
ILetOrUseExpr
FSharpTokenType+LetTokenElement(type:LET, text:let)
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
ILocalBinding
ILocalReferencePat
IExpressionReferenceName
FSharpIdentifierToken(type:IDENTIFIER, text:c)
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
FSharpTokenType+EqualsTokenElement(type:EQUALS, text:=)
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
ILiteralExpr
FSharpToken(type:INT32, text:1)
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
ISequentialExpr
IBinaryAppExpr
IBinaryAppExpr
IReferenceExpr
FSharpIdentifierToken(type:IDENTIFIER, text:c)
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
IReferenceExpr
FSharpIdentifierToken(type:PLUS, text:+)
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
IReferenceExpr
FSharpIdentifierToken(type:IDENTIFIER, text:c)
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
IReferenceExpr
FSharpIdentifierToken(type:SYMBOLIC_OP, text:|>)
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
IReferenceExpr
FSharpIdentifierToken(type:IDENTIFIER, text:ignore)
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
IUnitExpr
FSharpTokenType+LparenTokenElement(type:LPAREN, text:()
FSharpTokenType+RparenTokenElement(type:RPAREN, text:))
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
module Module

let a =
let {caret}c = 1
c + c |> ignore

()
Copy link
Collaborator

@auduchinok auduchinok Aug 11, 2020

Choose a reason for hiding this comment

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

The same as above:

letExpr
  binding
    expr
  seqExpr
    c + c |> ignore

    ()

Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
module Module

let a =
let b{caret} =
if true then
1
else
2

()
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
IFSharpImplFile
INamedModuleDeclaration
FSharpTokenType+ModuleTokenElement(type:MODULE, text:module)
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
FSharpIdentifierToken(type:IDENTIFIER, text:Module)
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
ILetModuleDecl
FSharpTokenType+LetTokenElement(type:LET, text:let)
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
ITopBinding
ITopReferencePat
IExpressionReferenceName
FSharpIdentifierToken(type:IDENTIFIER, text:a)
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
FSharpTokenType+EqualsTokenElement(type:EQUALS, text:=)
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
IChameleonExpression
ISequentialExpr
IBinaryAppExpr
IIfThenElseExpr
FSharpTokenType+IfTokenElement(type:IF, text:if)
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
ILiteralExpr
FSharpTokenType+TrueTokenElement(type:TRUE, text:true)
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
FSharpTokenType+ThenTokenElement(type:THEN, text:then)
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
ILiteralExpr
FSharpToken(type:INT32, text:1)
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
FSharpTokenType+ElseTokenElement(type:ELSE, text:else)
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
ILiteralExpr
FSharpToken(type:INT32, text:2)
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
IReferenceExpr
FSharpIdentifierToken(type:SYMBOLIC_OP, text:|>)
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
IReferenceExpr
FSharpIdentifierToken(type:IDENTIFIER, text:ignore)
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
Whitespace(type:WHITE_SPACE, text: ) spaces:" "
IUnitExpr
FSharpTokenType+LparenTokenElement(type:LPAREN, text:()
FSharpTokenType+RparenTokenElement(type:RPAREN, text:))
NewLine(type:NEW_LINE, text:\n) spaces:"\n"
Loading