Repository navigation
Add lint rule requiring SAFETY comments on unsafe operations - #65024
Open
Jake Bailey (jakebailey) wants to merge 11 commits into
Open
Jake Bailey (jakebailey) wants to merge 11 commits into
Jake Bailey (jakebailey) wants to merge 11 commits into
Conversation
Uses of package unsafe can silently break memory safety, and the invariants that make each one sound are easy to lose track of. Like Rust's SAFETY convention, require every unsafe operation to carry a nearby `// SAFETY:` comment justifying it, so reviewers and future editors can check the reasoning still holds. Sizeof, Alignof, and Offsetof are exempt since they are compile-time constants, as is naming unsafe.Pointer as a type.
The new unsafesafety lint rule requires every unsafe operation to be justified. The generated casts from *Node to concrete node structs are all sound for the same reason: each kind is only ever allocated as its own struct, which embeds the Node header at offset zero. Have the generator state that at each cast site.
Document the invariant each remaining unsafe operation relies on, as required by the unsafesafety lint rule, so the reasoning is visible at the use site and can be rechecked when surrounding code changes.
typescript-automation
Bot
added
Author: Team
For Uncommitted Bug
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The linter can silently allow unsafe operations without their own SAFETY justification by reusing trailing comments and skipping dot-imported calls and conversions from unsafe.Pointer.
3 open issues
What changed in this PR
Adds a custom Go lint rule requiring SAFETY: comments for unsafe operations, making memory-safety invariants explicit across the native compiler and filesystem code.
Changes:
- Registers the analyzer and adds fixtures with expected diagnostics.
- Documents existing unsafe operations without changing their runtime behavior.
- Updates the AST generator to emit safety comments for generated casts.
| File | Description |
|---|---|
tsc/internal/vfs/vfstest/vfstest.go |
Documents string-backed file-data immutability. |
tsc/internal/vfs/internal/internal.go |
Documents byte-to-string conversion invariants. |
tsc/internal/nativepath/symlink_windows.go |
Documents Windows attribute-buffer compatibility. |
tsc/internal/fswatch/windows.go |
Documents notification-buffer casts and attribute queries. |
tsc/internal/fswatch/walkdir_windows.go |
Documents directory attribute-buffer compatibility. |
tsc/internal/fswatch/walkdir_unix.go |
Documents directory-entry alignment and boundaries. |
tsc/internal/fswatch/inotify_linux.go |
Documents event-record layout assumptions. |
tsc/internal/fswatch/fsevents_darwin.go |
Documents native-call lifetimes and event-array access. |
tsc/internal/fswatch/fsevents_darwin_ffi.go |
Documents native-buffer ownership and pointer handling. |
tsc/internal/fswatch/fanotify_linux.go |
Documents metadata alignment and size assumptions. |
tsc/internal/checker/types.go |
Documents type-kind and embedding invariants for casts. |
tsc/internal/checker/mapper.go |
Documents mapper-kind and embedding invariants. |
tsc/internal/ast/ast.go |
Documents flow-node cast invariants. |
tools/scripts/tsc/generate-go-ast.ts |
Emits safety comments for generated node casts. |
tools/customlint/unsafesafety.go |
Implements unsafe-operation detection and comment matching. |
tools/customlint/testdata/unsafesafety/unsafesafety.go.golden |
Records expected analyzer diagnostics. |
tools/customlint/testdata/unsafesafety/unsafesafety.go |
Exercises accepted comments and missing-comment diagnostics. |
tools/customlint/plugin.go |
Registers the new analyzer. |
Review effort: Balanced
Review found ways for unsafe operations to slip through without their own justification: converting an unsafe.Pointer back to another type (including via aliases or named pointer types), calling unsafe through a dot import, and a trailing SAFETY comment on one statement silently covering the statement after it. Each of these needs its own stated invariant just as much as the forms already checked.
The unsafesafety rule now flags conversions from unsafe.Pointer, which caught these uintptr arguments to syscall_syscall6. They were never seen before because nothing lints the darwin build.
Several fswatch files are only built for darwin, the BSDs, or other Unix targets, and none of our lint runners compile them, so rule violations there go unnoticed. Cross-linting from Linux runners via GOOS covers them without needing a runner for each OS. Running all targets in one job took far too long, so each target gets its own matrix entry. The custom linter build ignores GOOS and GOARCH so the binary still runs on the host.
The protectedfield test from main runs every analyzer over its test data, so its unannotated cast now trips unsafesafety.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The lint check incorrectly rejects trailing SAFETY comments on multiline conversions and fails to flag undocumented unsafe-pointer conversions through type parameters.
2 open issues
3 resolved since last review
Review effort: Balanced
A trailing SAFETY comment after the closing paren of a multiline unsafe call is clearly about that call, but was rejected because only start lines were matched. Conversions through a type parameter constrained to unsafe.Pointer are just as unsafe as direct ones, but the parameter's underlying type is an interface, so they slipped through unflagged.
An alias of a type parameter hid the parameter, letting its unsafe conversions go unflagged. Embedded constraints were treated as a union rather than an intersection, so a constraint narrowed to uintptr still demanded a SAFETY comment. Boxing an unsafe.Pointer into an interface cannot break memory safety, yet it was flagged. That became much more visible once constraints like any were treated as possibly containing unsafe.Pointer.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The lint rule incorrectly flags safe generic identity conversions as unsafe, requiring unnecessary safety comments even when source and destination types are identical.
1 open issue
2 resolved since last review
Review effort: Balanced
Converting a value to its own type cannot break memory safety, but a generic T(x) with T constrained by any or comparable was flagged because those type sets include unsafe.Pointer.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Subtle interactions between generic constraint intersections and named-type identity require human assessment because they can cause safe conversions to be incorrectly flagged as unsafe.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We don't have much unsafe (well, besides the AST stuff), but I think it'd be worth forcing ourselves to write out invariants rust-style.