Fix abstract class insert location when first good member has attributes #1107
Conversation
…ncorrectly calculating from the first-valid
baronfel
force-pushed
the
fix-abstract-class-insert-pos-calculation
branch
from
8ae6b24 to
5670c0d
Compare
|
This ended up being a bit more like a general cleanup of abstract member generation - there was a lot of cruft that was no longer necessary thanks the to trivia that @nojaf added to the AST over the years. This was all very legacy code and got nicely streamlined. Along the way I re-enabled the generation tests we had and fixed the whitespace comparison issues that caused them to be disabled in the first place - the silent killer was IDE space-trimming in the 'expected' text blocks. |
| let furthestMemberToSkip, otherMembers = | ||
| ((inheritMemberRange, []), allMembers) | ||
| // find the last of the following kinds of members, as object-programming members must come after these | ||
| // * implicit/explicit constructors | ||
| // * `inherit` expressions | ||
| // * class-level `do`/`let` bindings (`do` bindings are actually `LetBindings` in the AST) | ||
| ||> List.fold (fun (m, otherMembers) memb -> | ||
| match memb with | ||
| | (SynMemberDefn.ImplicitCtor _ | ExplicitCtor | SynMemberDefn.ImplicitInherit _ | SynMemberDefn.LetBindings _) as possible -> | ||
| let c = Range.rangeOrder.Compare(m, possible.Range) | ||
|
|
||
| let m' = if c < 0 then possible.Range else m | ||
|
|
||
| m', otherMembers | ||
| | otherMember -> m, otherMember :: otherMembers) |
There was a problem hiding this comment.
This is the core change - we no longer insert before-the-last member, instead we find the first available insert location that's valid and insert there. Easier to reason about IMO.
| @@ -84,77 +98,62 @@ let private tryFindAbstractClassExprInParsedInput | |||
| let tryFindAbstractClassExprInBufferAtPos | |||
| (codeGenService: ICodeGenerationService) | |||
| (pos: Position) | |||
| (document: Document) | |||
| (document: NamedText) | |||
There was a problem hiding this comment.
This document structure is just a less-well-used NamedText - we should standardize on the new hotness.
| match memberNamesAndRanges with | ||
| | (_, _, leadingKeywordRange) :: _ -> | ||
| // if we have any members, then we can use the start of the leading keyword to give us the indent correctly | ||
| leadingKeywordRange.StartColumn | ||
| | [] -> | ||
| match abstractClassData with | ||
| | AbstractClassData.ExplicitImpl(inheritExpressionRange = inheritRange) -> | ||
| // 'interface ISomething with' is often in a new line, we use the indentation of that line | ||
| inheritRange.StartColumn | ||
| | AbstractClassData.ObjExpr(newExpression = newExpr; withKeyword = withKeyword; bindings = bindings) -> | ||
| // two cases here to consider: | ||
| // * has a with keyword on same line as newExpr | ||
| match withKeyword with | ||
| | None -> | ||
| // if no withKeyword, then we add an indent to the start of the new Expression to get our final indent | ||
| newExpr.StartColumn + indentSize | ||
| | Some keyword -> | ||
| // if we have a keyword, if it's on the same line then we can do the same | ||
| if keyword.StartLine = newExpr.StartLine then | ||
| newExpr.StartColumn + indentSize | ||
| else if keyword.StartColumn = newExpr.StartColumn then | ||
| keyword.StartColumn + indentSize | ||
| else | ||
| keyword.StartColumn | ||
|
|
There was a problem hiding this comment.
massive cleanup here thanks to AST additions - we can do all of these checks with absolute precision now. And synchronously!
| match Lexer.getSymbol range.Start.Line range.Start.Column lineText SymbolLookupKind.ByLongIdent [||] with | ||
| | Some sym -> | ||
| checkResultForFile.GetCheckResults.GetSymbolUseAtLocation( | ||
| range.StartLine, | ||
| range.EndColumn, | ||
| lineText, | ||
| sym.Text.Split('.') |> List.ofArray |
There was a problem hiding this comment.
getting the full 'ident islands' here fixed another bug - without this we'd never detect already-implemented members from an abstract class correctly, so our 'partial-fixup' story was broken.
| @@ -863,7 +863,7 @@ type FSharpConfig = | |||
| AbstractClassStubGeneration = defaultArg dto.AbstractClassStubGeneration false | |||
| AbstractClassStubGenerationObjectIdentifier = defaultArg dto.AbstractClassStubGenerationObjectIdentifier "this" | |||
| AbstractClassStubGenerationMethodBody = | |||
| defaultArg dto.AbstractClassStubGenerationMethodBody "failwith \Not Implemented\"" | |||
| defaultArg dto.AbstractClassStubGenerationMethodBody "failwith \"Not Implemented\"" | |||
There was a problem hiding this comment.
this was another bug that caused invalid generation.
| @@ -114,7 +114,7 @@ module CodeFix = | |||
| (beforeWithCursor: string) | |||
| (validateDiagnostics: Diagnostic[] -> unit) | |||
| (chooseFix: ChooseFix) | |||
| (expected: ExpectedResult) | |||
| (expected: unit -> ExpectedResult) | |||
There was a problem hiding this comment.
this was to delay work done at test-app-startup time - I wanted to debug the comparison and every test was calling this.
Previously when completing an abstract class we'd look for the first non-ctor/interface member and go back a line + indent. That doesn't work when attributes are in play. Instead, now we look for the last invalid member - which are implicit ctors, explicit ctors, class-level let/do bindings, and inherit expressions - and insert the line after that + indent.
WHAT
🤖 Generated by Copilot at 8ae6b24
Refactor
walkTypeDefnfunction inAbstractClassStubGenerator.fsto simplify logic and improve stub insertion position. This fixes a bug in the stub generator for abstract classes.🤖 Generated by Copilot at 8ae6b24
🐛🧹🚀
WHY
HOW
🤖 Generated by Copilot at 8ae6b24
TODO
Test cases for the scenario reported by @lenscas on discord.