Add lint rule requiring SAFETY comments on unsafe operations

.github/workflows/ci.yml

@@ -254,6 +254,27 @@ jobs:
             name: 'noembed'
             noembed: true
             skip: ${{ github.event_name == 'merge_group' }}
+          # Cross-lint OSes with build-tagged files we have no runner for.
+          - runner: ubuntu-latest
+            name: 'darwin'
+            goos: darwin
+            goarch: arm64
+            skip: ${{ github.event_name == 'merge_group' }}
+          - runner: ubuntu-latest
+            name: 'freebsd'
+            goos: freebsd
+            goarch: amd64
+            skip: ${{ github.event_name == 'merge_group' }}
+          - runner: ubuntu-latest
+            name: 'dragonfly'
+            goos: dragonfly
+            goarch: amd64
+            skip: ${{ github.event_name == 'merge_group' }}
+          - runner: ubuntu-latest
+            name: 'solaris'
+            goos: solaris
+            goarch: amd64
+            skip: ${{ github.event_name == 'merge_group' }}
         exclude:
           - config:
               skip: true
@@ -276,6 +297,9 @@ jobs:
           echo "::remove-matcher owner=go::"
       - run: npm ci
       - run: npx hereby lint
+        env:
+          GOOS: ${{ matrix.config.goos }}
+          GOARCH: ${{ matrix.config.goarch }}
 
   format:
     runs-on: ubuntu-slim

Herebyfile.mjs

@@ -1239,7 +1239,8 @@ const customLinterTool = createCachedTool({
     ],
     exclude: ["**/testdata/**"],
     build: async toolPath => {
-        await run("go", ["run", golangciLintPackage(), "custom"]);
+        // GOOS/GOARCH select the lint target, but the linter must run on the host.
+        await run("go", ["run", golangciLintPackage(), "custom"], { env: { GOOS: "", GOARCH: "" } });
         await run(toolPath, ["cache", "clean"]);
     },
 });

tools/customlint/plugin.go

@@ -24,6 +24,7 @@ func (f *plugin) BuildAnalyzers() ([]*analysis.Analyzer, error) {
 		shadowAnalyzer,
 		typedPathsAnalyzer,
 		unexportedAPIAnalyzer,
+		unsafeSafetyAnalyzer,
 	}, nil
 }
 

tools/customlint/testdata/protectedfield/record/record.go

@@ -43,6 +43,7 @@ func (h *Header) AsLarge() *Large {
 	if h.Kind != KindLarge {
 		panic("bad cast")
 	}
+	// SAFETY: Kind is KindLarge, so h is the Header embedded at the start of a Large.
 	return (*Large)(unsafe.Pointer(h))
 }
 

tools/customlint/testdata/protectedfield/record/record.go.golden

@@ -45,6 +45,7 @@
 		if h.Kind != KindLarge {
 			panic("bad cast")
 		}
+		// SAFETY: Kind is KindLarge, so h is the Header embedded at the start of a Large.
 		return (*Large)(unsafe.Pointer(h))
 	}
 

tools/customlint/testdata/unsafesafety/dotimport.go

@@ -0,0 +1,14 @@
+package unsafesafety
+
+import . "unsafe"
+
+func badDotImport(h *header, s string) {
+	_ = (*derived)(Pointer(h))
+	_ = Slice(StringData(s), len(s))
+}
+
+func goodDotImport(h *header) {
+	// SAFETY: Dot imports are checked the same way.
+	_ = (*derived)(Pointer(h))
+	_ = Sizeof(*h)
+}

tools/customlint/testdata/unsafesafety/dotimport.go.golden

@@ -0,0 +1,21 @@
+	package unsafesafety
+
+	import . "unsafe"
+
+	func badDotImport(h *header, s string) {
+		_ = (*derived)(Pointer(h))
+		               ~~~~~~~~~~
+!!! unsafesafety: conversion to unsafe.Pointer must be preceded by a `// SAFETY:` comment explaining why it is sound
+		_ = Slice(StringData(s), len(s))
+		    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+!!! unsafesafety: use of unsafe.Slice must be preceded by a `// SAFETY:` comment explaining why it is sound
+		          ~~~~~~~~~~~~~
+!!! unsafesafety: use of unsafe.StringData must be preceded by a `// SAFETY:` comment explaining why it is sound
+	}
+
+	func goodDotImport(h *header) {
+		// SAFETY: Dot imports are checked the same way.
+		_ = (*derived)(Pointer(h))
+		_ = Sizeof(*h)
+	}
+

tools/customlint/testdata/unsafesafety/unsafesafety.go

@@ -0,0 +1,178 @@
+package unsafesafety
+
+import "unsafe"
+
+type header struct {
+	kind int
+}
+
+type derived struct {
+	header
+	value int
+}
+
+func bad(h *header, b []byte, s string) {
+	_ = (*derived)(unsafe.Pointer(h))
+	_ = unsafe.String(&b[0], len(b))
+	_ = unsafe.Slice(unsafe.StringData(s), len(s))
+	_ = unsafe.SliceData(b)
+	_ = unsafe.Add(unsafe.Pointer(h), 1)
+
+	// Not a safety comment.
+	_ = (*derived)(unsafe.Pointer(h))
+
+	// SAFETY: Separated from the use by a blank line.
+
+	_ = (*derived)(unsafe.Pointer(h))
+
+	// safety: Wrong case.
+	_ = (*derived)(unsafe.Pointer(h))
+
+	if h.kind == 1 {
+		_ = (*derived)(unsafe.Pointer(h))
+	}
+
+	_ = unsafe.Pointer(h) // SAFETY: Trailing comments only cover their own line.
+	_ = unsafe.Pointer(h)
+}
+
+type ptrAlias = unsafe.Pointer
+
+type ptrNamed unsafe.Pointer
+
+func badConversions(p unsafe.Pointer, a ptrAlias, n ptrNamed, h *header) {
+	_ = (*int)(p)
+	_ = uintptr(p)
+	_ = (*int)(a)
+	_ = (*int)(n)
+	_ = ptrAlias(h)
+	_ = ptrNamed(h)
+}
+
+// SAFETY: Comments on a function do not cover its body.
+func badFuncComment(h *header) *derived {
+	return (*derived)(unsafe.Pointer(h))
+}
+
+func good(h *header, b []byte, p unsafe.Pointer) {
+	// SAFETY: h is always embedded in a derived.
+	_ = (*derived)(unsafe.Pointer(h))
+
+	_ = (*derived)(unsafe.Pointer(h)) // SAFETY: Trailing comment.
+
+	/* SAFETY: Block comment. */
+	_ = (*derived)(unsafe.Pointer(h))
+
+	// Multiple lines of explanation are fine,
+	// SAFETY: as long as one starts with SAFETY.
+	// More text.
+	_ = (*derived)(unsafe.Pointer(h))
+
+	// SAFETY: One comment covers the whole statement.
+	_ = unsafe.Slice((*byte)(unsafe.Pointer(h)), unsafe.Sizeof(*h))
+
+	// SAFETY: Covers the if statement, including its init.
+	if p := (*derived)(unsafe.Pointer(h)); p != nil {
+		_ = p
+	}
+
+	use(
+		1,
+		// SAFETY: Comment directly above an argument.
+		unsafe.Pointer(h),
+	)
+
+	switch h.kind {
+	case 1:
+		// SAFETY: Inside a case clause.
+		_ = (*derived)(unsafe.Pointer(h))
+	}
+
+	// SAFETY: Conversions from unsafe.Pointer need justification too.
+	_ = (*derived)(p)
+
+	// Compile-time operations are always safe.
+	_ = unsafe.Sizeof(*h)
+	_ = unsafe.Alignof(*h)
+	_ = unsafe.Offsetof(derived{}.value)
+}
+
+// Using unsafe.Pointer as a type is not itself unsafe.
+func use(_ int, p unsafe.Pointer) unsafe.Pointer {
+	return p
+}
+
+// SAFETY: Package-level declarations are covered too.
+var global = unsafe.Pointer(&derived{})
+
+var badGlobal = unsafe.Pointer(&derived{})
+
+func multiline(h *header) {
+	_ = unsafe.Pointer(
+		h,
+	) // SAFETY: Trailing the end of a multiline use.
+
+	if (*derived)(unsafe.Pointer(h)) != nil {
+	} // SAFETY: Trailing a statement's end does not cover its start.
+}
+
+func fromTypeParam[P ~unsafe.Pointer](p P) *int {
+	return (*int)(p)
+}
+
+func toTypeParam[P ~unsafe.Pointer](p *int) P {
+	return P(p)
+}
+
+type pointerish interface {
+	~unsafe.Pointer | *int
+}
+
+func embeddedConstraint[P pointerish](p *int) P {
+	// SAFETY: Type parameters whose type set includes unsafe.Pointer count.
+	return P(p)
+}
+
+func notPointerTypeParam[P ~*int](p *int) P {
+	return P(p)
+}
+
+func aliasedTypeParam[P ~unsafe.Pointer](p *int, q P) {
+	type Alias = P
+	_ = Alias(p)
+	var a Alias = q
+	_ = (*int)(a)
+}
+
+type onlyUintptr interface {
+	~unsafe.Pointer | ~uintptr
+	~uintptr
+}
+
+func intersectedConstraint[P onlyUintptr](p P) uintptr {
+	return uintptr(p)
+}
+
+type ptrOrUintptr interface {
+	~unsafe.Pointer | ~uintptr
+}
+
+type diamond interface {
+	ptrOrUintptr
+	interface{ ptrOrUintptr }
+}
+
+func diamondConstraint[P diamond](p P) uintptr {
+	return uintptr(p)
+}
+
+func boxing[P ~unsafe.Pointer](p P, q unsafe.Pointer) {
+	_ = any(p)
+	_ = any(q)
+}
+
+func identity[T any, C comparable](x T, c C, p unsafe.Pointer) {
+	_ = T(x)
+	_ = C(c)
+	_ = unsafe.Pointer(p)
+}