Fix for a snapshot update with a nil CommandLine#3271
Fix for a snapshot update with a nil CommandLine#3271navya9singh wants to merge 3 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes an API panic in NewProjectResponse when a project.Project has a nil CommandLine, which can occur during snapshot updates while exercising code actions.
Changes:
- Add a nil check around
p.CommandLineaccess inNewProjectResponse. - Return
RootFiles/CompilerOptionsfrom locals populated only whenCommandLineis present.
internal/api/proto.go
Outdated
| var rootFiles []string | ||
| var opts *core.CompilerOptions | ||
| if p.CommandLine != nil { | ||
| rootFiles = p.CommandLine.FileNames() | ||
| opts = p.CommandLine.CompilerOptions() | ||
| } | ||
| return &ProjectResponse{ | ||
| Id: ProjectHandle(p), | ||
| ConfigFileName: p.Name(), | ||
| RootFiles: p.CommandLine.FileNames(), | ||
| CompilerOptions: p.CommandLine.CompilerOptions(), | ||
| RootFiles: rootFiles, | ||
| CompilerOptions: opts, | ||
| } |
There was a problem hiding this comment.
When p.CommandLine is nil, rootFiles stays a nil slice and opts stays nil. With the JSON encoder, these will serialize as null for rootFiles/compilerOptions, but the TypeScript API typings treat both as required (non-null) fields. Consider defaulting to an empty slice ([]) and an empty CompilerOptions object to preserve the response shape while still avoiding the panic.
| func NewProjectResponse(p *project.Project) *ProjectResponse { | ||
| var rootFiles []string | ||
| var opts *core.CompilerOptions | ||
| if p.CommandLine != nil { | ||
| rootFiles = p.CommandLine.FileNames() | ||
| opts = p.CommandLine.CompilerOptions() | ||
| } | ||
| return &ProjectResponse{ | ||
| Id: ProjectHandle(p), | ||
| ConfigFileName: p.Name(), | ||
| RootFiles: p.CommandLine.FileNames(), | ||
| CompilerOptions: p.CommandLine.CompilerOptions(), | ||
| RootFiles: rootFiles, | ||
| CompilerOptions: opts, | ||
| } |
There was a problem hiding this comment.
This fixes a panic path (nil CommandLine) but there’s no regression test to prevent it from coming back. Consider adding a small API/proto test that creates a project via project.Session.APIOpenProject, sets proj.CommandLine = nil, and asserts NewProjectResponse doesn’t panic (and that the JSON shape matches the TS API contract).
|
When does a project have a nil CommandLine? |
Found this while testing code actions on the typescript repo. NewProjectResponse panics when a project has a nil CommandLine. It was hard to add a test for this, but adding a nil check fixed the panic.