-
Notifications
You must be signed in to change notification settings - Fork 57
AI/Junie: update context collection mechanism with new schema #940
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
Conversation
e41f70a to
8870d07
Compare
| public static string GetSourceName([CanBeNull] this IIdentifier identifier) | ||
| { | ||
| if (identifier == null) return SharedImplUtil.MISSING_DECLARATION_NAME; | ||
| return identifier.PrevSibling?.GetTokenType() == FSharpTokenType.QUOTE ? "'" + identifier.Name : identifier.Name; |
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.
I think it should be part of a IFSharpIdentifier implementation. We should probably not assume a particular tree structure here.
| <Compile Include="Refactorings\IntroduceVarTest.fs" /> | ||
| <Compile Include="Refactorings\InlineVarTest.fs" /> | ||
| <Compile Include="TypeProviders\PrimitiveTypesBoxerTest.fs" /> | ||
| <Compile Include="AICore\FSharpFileSummarizerTest.fs" /> |
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.
What does the Core part mean in the context of the plugin tests? Is it going to be somehow different from other AI features?
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.
To be fair, this is a unit test that tests an implementation detail, not a feature
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.
renamed
| type private FileSummarizerVisitor() = | ||
| inherit TreeNodeVisitor<SummarizerContext>() | ||
|
|
||
| static let displayContext = emptyDisplayContext |
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.
Why is this binding needed?
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.
It's just a single DisplayContext for the entire FileSummarizerVisitor implementation which currently uses the default context. If we want to change the default behavior, it will be enough to change this value, rather than replace all emptyDisplayContext in this implementation.
| var members = TypeRepresentation switch | ||
| { | ||
| null => TypeMembersEnumerable, | ||
| IClassRepresentation classRepr => classRepr.TypeMembersEnumerable, | ||
| IInterfaceRepresentation interfaceRepr => interfaceRepr.TypeMembersEnumerable, | ||
| _ => TreeNodeEnumerable<ITypeBodyMemberDeclaration>.Empty | ||
| }; |
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.
Do we need it as a part of PSI? Are there any other features, besides the AI summarization, that need it?
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.
I thought it might be useful. It's not used anywhere else except AI right now.
It also complements the existing TypeInheritMember API (which also seems to be used in only one feature)
Lines 11 to 14 in e39c3ec
| public partial interface IFSharpTypeDeclaration | |
| { | |
| [CanBeNull] ITypeInherit TypeInheritMember { get; } | |
| } |
resharper-fsharp/ReSharper.FSharp/src/FSharp/FSharp.Psi/src/Impl/Tree/FSharpTypeDeclaration.cs
Lines 35 to 49 in e39c3ec
| public ITypeInherit TypeInheritMember | |
| { | |
| get | |
| { | |
| ITypeInherit TryFindTypeInherit(TreeNodeEnumerable<ITypeBodyMemberDeclaration> memberDecls) => | |
| memberDecls.OfType<ITypeInherit>().SingleItem(); | |
| return TypeRepresentation switch | |
| { | |
| null => TryFindTypeInherit(TypeMembersEnumerable), | |
| IClassRepresentation classRepr => TryFindTypeInherit(classRepr.TypeMembersEnumerable), | |
| _ => null | |
| }; | |
| } | |
| } |
But we can also leave it as the details of the AI Summarization implementation.
| exception E1 of string | ||
| with member x.NewMessage = "" | ||
|
|
||
| type Record = { |
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.
Please try to split this file into multiple ones.
| let typeRepr = referencePat.GetFcsSymbol() |> formatFcsSymbolType | ||
| let representation = $"val {referencePat.GetFcsSymbol().DisplayName}: {typeRepr}" | ||
| context.AddEntity(binding, representation) |
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.
These tree lines are repeated several times, with the only difference being the representation string. Could you try extracting it?
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.
In fact, in all three cases the common code consists only in calling
context.AddEntity(binding, representation)The rest of the code constructs a different from each other representation
let typeRepr = referencePat.GetFcsSymbol() |> formatFcsSymbolType
let representation = $"val {referencePat.GetFcsSymbol().DisplayName}: {typeRepr}"
context.AddEntity(binding, representation)let typeRepr = constructor.GetFcsSymbol() |> formatFcsSymbolType
let representation = $"new: {typeRepr}"
context.AddEntity(constructor, representation)let representation = $"member {memberDecl.SourceName}: {typeRepr}" + accessors
context.AddEntity(memberDecl, representation)I'm not sure if there's anything can be done here, except to inline some variables.
|
|
||
| override x.VisitNamedModuleDeclaration(moduleDecl, context) = | ||
| // Currently, we're not adding a top-level module to the scope to not duplicate its name in tokens | ||
| context.AddEntity(moduleDecl, "module " + moduleDecl.ClrName) |
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.
Are we sure we want to use Clr name instead of the F# source name?
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.
SourceName is not suitable, as we need a qualified name. Perhaps it's worth adding something like QualifiedName to INamedModuleDeclaration?
| use _ = context.OpenScope(interfaceImpl, $"interface {interfaceRepr}") | ||
| x.VisitNode(interfaceImpl, context) | ||
|
|
||
| override x.VisitTopBinding(binding, context) = |
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 should really try to fix the hierarchy instead of adding more repeating code like this :(
ReSharper.FSharp/src/FSharp/FSharp.Psi/src/Impl/FSharpImplUtil.cs
Outdated
Show resolved
Hide resolved
ReSharper.FSharp/src/FSharp/FSharp.Psi/src/Impl/FSharpImplUtil.cs
Outdated
Show resolved
Hide resolved
fe6c6d6 to
d9d5465
Compare
c1cc6aa to
ae951f8
Compare
GitOrigin-RevId: 6d04adf # Conflicts: # dotnet/Plugins/resharper-fsharp/ReSharper.FSharp/src/FSharp/FSharp.Psi.Daemon/src/Stages/InferredTypeCodeVisionProvider.fs # dotnet/Plugins/resharper-fsharp/ReSharper.FSharp/src/FSharp/FSharp.Psi.Services/src/Util/FcsTypeUtil.fs GitOrigin-RevId: 68b10107ed9fc1f5e0a3bcadfa812d35f2d475cf
GitOrigin-RevId: 6d04adf # Conflicts: # dotnet/Plugins/resharper-fsharp/ReSharper.FSharp/src/FSharp/FSharp.Psi.Daemon/src/Stages/InferredTypeCodeVisionProvider.fs # dotnet/Plugins/resharper-fsharp/ReSharper.FSharp/src/FSharp/FSharp.Psi.Services/src/Util/FcsTypeUtil.fs GitOrigin-RevId: 68b10107ed9fc1f5e0a3bcadfa812d35f2d475cf
No description provided.