Skip to content

Fix file rename crashes when dealing with solution configuration files#4067

Open
DanielRosenwasser wants to merge 5 commits into
mainfrom
renameWithNoFileSolutionConfig
Open

Fix file rename crashes when dealing with solution configuration files#4067
DanielRosenwasser wants to merge 5 commits into
mainfrom
renameWithNoFileSolutionConfig

Conversation

@DanielRosenwasser
Copy link
Copy Markdown
Member

Fixes #4066.

Copilot AI review requested due to automatic review settings May 28, 2026 01:10
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes a nil-pointer panic during workspace/willRenameFiles handling when a “solution-style” ancestor tsconfig.json project exists without a built Program (e.g., it only references child composite projects). This makes file renames safe in multi-project/solution configurations in the language server.

Changes:

  • Guard LanguageService.GetEditsForFileRename against a nil Program to prevent crashes.
  • Add a fourslash regression test that exercises renaming within a referenced composite project while an ancestor solution project has a nil program.
Show a summary per file
File Description
internal/ls/file_rename.go Adds a nil-check for program before computing rename edits, preventing a panic.
internal/fourslash/tests/getEditsForFileRenameSolutionNoCrash_test.go Adds regression coverage for solution-style tsconfig rename behavior to ensure no crash and correct import update.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 0


func (l *LanguageService) GetEditsForFileRename(ctx context.Context, oldURI lsproto.DocumentUri, newURI lsproto.DocumentUri) []lsproto.TextDocumentEditOrCreateFileOrRenameFileOrDeleteFile {
program := l.GetProgram()
if program == nil {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is ok, but for correctness, we probably also need to make sure that we receive the project's command line here so that we can properly rename things in tsconfigs even when the program is nil.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I assumed that a program could only be nil if we ended up with no files - in which case, there should be nothing to update, right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could still have to update paths in its tsconfig I think, which is what we try to do a few lines below here in l.updateTsconfigFiles.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm having a hard time wrapping my head around when a tsconfig.json may not have an associated Program, but the owning project will have a CommandLine set.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@gabritto and I talked about this off-thread.

I can imagine that maybe there's scenarios where there is a ParsedCommandLine but no Program, but not where we would care to make edits in those files.

@gabritto did mention there may be issues where we are too lazy and don't actually open up related projects from the solution file, but none of the recent fixes do that.

I think if we have a specific scenario that isn't working, we should file a bug and amend the logic. Until then, I'll revert the latest commit and we can get in the latest logic to avoid the crash.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm more surprised that we can create a LanguageService without a program... would it work to guard against that at a higher level, or do we need this for something?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I am also surprised, but there is no precedent for returning nil for any *LanguageService unless some error has occurred.

So short of that, it feels like the cleanest and most reasonable alternative is to say that the nil guard belongs in GetLanguageServicesForDocuments (or maybe we need some sort of better guard to avoid creating Projects with no programs, but I have no idea where to start with that).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Panic on file rename with solution-style tsconfig.json

4 participants