Skip to content

Conversation

@jakebailey
Copy link
Member

I'd like to be able to cache stuff on CompilerOptions (e.g., source affecting options, maybe even the more complicated Get methods), however that will require sync.Mutex, sync.Once, etc. In order for that all to work, we must never copy CompilerOptions by value.

Adding any of those sync structs to CompilerOptions will make go vet error out (correctly) for the copies. But, it's clearer to preemptively fix all of those problems now so that later PRs don't have to do it.

A new Clone method makes a shallow copy of CompilerOptions; it's not "fast", but it's only used in testing. All other copies of CompilerOptions were simple to fix.


Another restriction with caching stuff on CompilerOptions is that CompilerOptions is treated as immutable, never changing after creation. Thankfully we do that already, but it's hard to check that sort of thing statically.

Copy link
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

This PR marks CompilerOptions as non‐copyable by embedding a noCopy field and updates all usages to work with pointers. Changes include adding nil checks and updating test and transformer files to use CompilerOptions pointers instead of values.

  • Updated test files and transformers to ensure CompilerOptions is never copied.
  • Added a Clone method using reflection in core/compileroptions.go and updated related call sites.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
internal/tsoptions/parsinghelpers_test.go Added check to skip unexported fields in iteration.
internal/tsoptions/decls_test.go Added check to skip unexported fields when iterating over fields.
internal/tsoptions/commandlineparser_test.go Updated handling of CompilerOptions to use pointers and nil checks.
internal/transformers/esmodule_test.go Converted CompilerOptions to pointer; added nil check.
internal/transformers/commonjsmodule_test.go Converted CompilerOptions to pointer; added nil check.
internal/testutil/harnessutil/harnessutil.go Updated CompilerOptions usage to work with pointers and Clone().
internal/core/compileroptions.go Embedded noCopy and added a reflection-based Clone method.

@jakebailey jakebailey requested a review from sheetalkamat June 9, 2025 19:30
@jakebailey jakebailey enabled auto-merge June 9, 2025 19:44
@jakebailey jakebailey added this pull request to the merge queue Jun 9, 2025
Merged via the queue into main with commit fdd1de5 Jun 9, 2025
23 checks passed
@jakebailey jakebailey deleted the jabaile/compiler-options-nocopy branch June 9, 2025 19:57
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.

3 participants