Skip to content

Fix namespace auto open if parsed input is empty #5040

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
KevinRansom merged 1 commit into from
Jun 1, 2018
Merged

Fix namespace auto open if parsed input is empty #5040

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
9 changes: 6 additions & 3 deletions src/fsharp/service/ServiceAssemblyContent.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -999,8 +999,11 @@ module ParsedInput =

mkPos line ctx.Pos.Column

let tryFindNearestPointToInsertOpenDeclaration (currentLine: int) (ast: ParsedInput) (entity: Idents) (insertionPoint: OpenStatementInsertionPoint) =
let findNearestPointToInsertOpenDeclaration (currentLine: int) (ast: ParsedInput) (entity: Idents) (insertionPoint: OpenStatementInsertionPoint) =
match tryFindNearestPointAndModules currentLine ast insertionPoint with
| Some (scope, _, point), modules ->
Some (findBestPositionToInsertOpenDeclaration modules scope point entity)
| _ -> None
findBestPositionToInsertOpenDeclaration modules scope point entity
| _ ->
// we failed to find insertion point because ast is empty for some reason, return top left point in this case
Copy link
Contributor

@TIHan TIHan May 31, 2018

Choose a reason for hiding this comment

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

Do we want to do this anytime there is an empty ast?
What if we have a comment at the top of the file with open declarations below it? Would that mean the new open declaration will go above the comment?

Copy link
Contributor

@TIHan TIHan May 31, 2018

Choose a reason for hiding this comment

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

Also, could we find out why the ast is empty?

Copy link
Contributor Author

@vasily-kirichenko vasily-kirichenko Jun 1, 2018

Choose a reason for hiding this comment

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

What if we have a comment at the top of the file with open declarations below it? Would that mean the new open declaration will go above the comment?

Yes. Or maybe on top of the comment. Why?

Also, could we find out why the ast is empty?

Why? We cannot find a position to insert the open statement, that's enough. Or you suggest to fix the parser?..

Copy link
Contributor

@KevinRansom KevinRansom Jun 1, 2018

Choose a reason for hiding this comment

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

@TIHan

I looked at this, and it seems like the right fix to me, tryFindNearestPointAndModules is looking for the nearest open so that it can insert the new one below it, and finds none.

In findNearestPointToInsertOpenDeclaration when no open was found it quite reasonably says. Oh well, I couldn't find anywhere to insert the open, I guess I will use the top of the file.

It might be interesting to skip over leading comments, but that is a new feature and is not the intent of this PR.

Copy link
Contributor

@TIHan TIHan Jun 1, 2018

Choose a reason for hiding this comment

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

Skipping over comments would be not be trivial as we don't retain comments in the AST.

The reasoning why I mentioned it was because I thought the AST was giving us nothing even if we had open declarations, and we would just shove a open declaration at the top of the others, potentially skipping a comment if there was one there. However, I misinterpreted this. The case he is fixing is when there are no open declarations, which makes this fix absolutely fine.

Copy link
Contributor

@TIHan TIHan Jun 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@vasily-kirichenko Thank you for your continued work.

In the future, I recommend being more descriptive in the PRs rather than only showing a GIF. A GIF is great, but we need context.

{ ScopeKind = ScopeKind.TopModule
Pos = mkPos 1 0 }
2 changes: 1 addition & 1 deletion src/fsharp/service/ServiceAssemblyContent.fsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ module public ParsedInput =
(( (* requiresQualifiedAccessParent: *) Idents option * (* autoOpenParent: *) Idents option * (* entityNamespace *) Idents option * (* entity: *) Idents) -> (Entity * InsertContext)[])

/// Returns `InsertContext` based on current position and symbol idents.
val tryFindNearestPointToInsertOpenDeclaration : currentLine: int -> ast: Ast.ParsedInput -> entity: Idents -> insertionPoint: OpenStatementInsertionPoint -> InsertContext option
val findNearestPointToInsertOpenDeclaration : currentLine: int -> ast: Ast.ParsedInput -> entity: Idents -> insertionPoint: OpenStatementInsertionPoint -> InsertContext

/// Returns lond identifier at position.
val getLongIdentAt : ast: Ast.ParsedInput -> pos: Range.pos -> Ast.LongIdent option
Expand Down
2 changes: 1 addition & 1 deletion vsintegration/src/FSharp.Editor/Completion/CompletionProvider.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ type internal FSharpCompletionProvider
if Settings.CodeFixes.AlwaysPlaceOpensAtTopLevel then OpenStatementInsertionPoint.TopLevel
else OpenStatementInsertionPoint.Nearest

let! ctx = ParsedInput.tryFindNearestPointToInsertOpenDeclaration line.LineNumber parsedInput fullNameIdents insertionPoint
let ctx = ParsedInput.findNearestPointToInsertOpenDeclaration line.LineNumber parsedInput fullNameIdents insertionPoint
let finalSourceText, changedSpanStartPos = OpenDeclarationHelper.insertOpenDeclaration textWithItemCommitted ctx ns
let fullChangingSpan = TextSpan.FromBounds(changedSpanStartPos, item.Span.End)
let changedSpan = TextSpan.FromBounds(changedSpanStartPos, item.Span.End + (finalSourceText.Length - sourceText.Length))
Expand Down