bridgelint: check avatar hashes, media info, GIF capabilities and dropped decode errors

bridgelint/README.md

@@ -259,6 +259,7 @@ as deliberate for LP002, and sets `com.beeper.can_retry` for clients.
 | EP001 | error | Errors matched by exact string: `err.Error() == "..."`, `switch err.Error()`, `strings.EqualFold(err.Error(), ...)`. Comparisons with `""` and substring matches aren't flagged, since some servers only give text. |
 | EP002 | warning | In library packages (ones that don't import bridgev2, other than `main`), an `http.Response` status formatted into a plain error, so callers can't tell 429 from 403 from 5xx. Forks like fhttp count when their `Response` has net/http's fields. |
 | EP003 | info | `errors.Join` returned from a Matrix handler or used as a user-facing message. |
+| EP004 | warning | In a function with no results, `if json.Unmarshal(...) != nil { return }`, the same with `err := ...; err != nil`, or a `json.Decoder.Decode`/`proto.Unmarshal` check, where the branch is only a bare `return`. A payload of an unexpected shape then disappears with no log. `continue` isn't flagged, since loops often skip entries of other shapes on purpose. |
 
 ### General reliability (GR)
 
@@ -274,6 +275,16 @@ as deliberate for LP002, and sets `com.beeper.can_retry` for clients.
 | GR011 | warning | `GetCapabilities` changes `RoomFeatures` fields but never the `ID`, so clients cache the wrong features. |
 | GR013 | warning | `LogoutRemote` that never disconnects or stops the client, when the pinned mautrix doesn't disconnect after `LogoutRemote` itself (v0.32.0 and earlier). The stopping call is recognised by name, a name heuristic. |
 
+### Matrix content (MC)
+
+What clients need from bridged events and room features to render them correctly.
+
+| Rule | Severity | Flags |
+|---|---|---|
+| MC001 | error | A `bridgev2.Avatar` built in the function with a non-constant `MXC` and no `Hash`, either as a literal or by assigning `MXC` to an avatar literal that has no hash. bridgev2 skips the update when the new hash equals the stored one and an MXC is already set, so every avatar change after the first is lost. Avatars passed in or returned by helpers aren't checked. |
+| MC002 | warning | Image capabilities that accept `image/gif` (a `FileFeatureMap` literal's `m.image` entry, following `MimeTypes` to a package-level map) when nothing in the package uses `event.CapMsgGIF`. Clients only enable their GIF picker with `fi.mau.gif`. |
+| MC003 | warning | Image, video or audio `MessageEventContent` built in the function, with a URL or file, that gets no `Info` on any path to the end of the function. Content handed to another function is assumed to get its info there. Clients use `info.mimetype` to pick a renderer and to name cached files. |
+
 ### Deliberately not included
 
 These were considered and left out because they can't be checked without false positives:

bridgelint/rules/analyzers.go

@@ -3,4 +3,4 @@ package rules
 import "golang.org/x/tools/go/analysis"
 
 // Analyzers are the bridgelint rule groups, in the order they're documented.
-var Analyzers = []*analysis.Analyzer{MSSAnalyzer, BSAnalyzer, LPAnalyzer, DMAnalyzer, EPAnalyzer, GRAnalyzer, BLAnalyzer}
+var Analyzers = []*analysis.Analyzer{MSSAnalyzer, BSAnalyzer, LPAnalyzer, DMAnalyzer, EPAnalyzer, GRAnalyzer, MCAnalyzer, BLAnalyzer}

bridgelint/rules/ep.go

@@ -10,7 +10,7 @@ import (
 
 var EPAnalyzer = &analysis.Analyzer{
 	Name: "ep",
-	Doc:  "error plumbing rules (EP001-EP003): errors must be matched by type and carry typed statuses",
+	Doc:  "error plumbing rules (EP001-EP004): errors must be matched by type, carry typed statuses and not vanish",
 	Run:  runEP,
 }
 
@@ -34,6 +34,10 @@ func runEP(pass *analysis.Pass) (any, error) {
 		}
 		ast.Inspect(f, func(n ast.Node) bool {
 			switch x := n.(type) {
+			case *ast.FuncDecl, *ast.FuncLit:
+				if sig := funcType(info, x); sig != nil && sig.Results().Len() == 0 {
+					checkDroppedDecode(pass, funcBody(x))
+				}
 			case *ast.BinaryExpr:
 				if (x.Op == token.EQL || x.Op == token.NEQ) && (isErrText(x.X) && !isEmptyString(info, x.Y) || isErrText(x.Y) && !isEmptyString(info, x.X)) {
 					report(pass, x, EP001, "error matched by comparing err.Error(); use errors.Is/errors.As or a typed code accessor")
@@ -90,3 +94,76 @@ func containsErrorsJoin(pass *analysis.Pass, e ast.Expr) bool {
 	})
 	return found
 }
+
+func isDecodeCall(info *types.Info, e ast.Expr) bool {
+	call, ok := unparen(e).(*ast.CallExpr)
+	if !ok {
+		return false
+	}
+	fn, ok := calleeObj(info, call).(*types.Func)
+	if !ok {
+		return false
+	}
+	if sig, _ := fn.Type().(*types.Signature); sig != nil && sig.Recv() != nil {
+		return fn.Name() == "Decode" && isNamed(sig.Recv().Type(), "encoding/json", "Decoder")
+	}
+	return isPkgFunc(fn, "encoding/json", "Unmarshal") || isPkgFunc(fn, "google.golang.org/protobuf/proto", "Unmarshal")
+}
+
+// checkDroppedDecode implements EP004: in a function that can't return the error, a failed decode that
+// only returns leaves no trace, so a payload shape the code didn't expect looks like a message that never
+// arrived. Only bare returns are flagged: continue usually skips entries of other shapes on purpose.
+func checkDroppedDecode(pass *analysis.Pass, body *ast.BlockStmt) {
+	info := pass.TypesInfo
+	inspectFunc(body, func(n ast.Node) bool {
+		ifs, ok := n.(*ast.IfStmt)
+		if !ok || ifs.Else != nil || len(ifs.Body.List) != 1 {
+			return true
+		}
+		if ret, ok := ifs.Body.List[0].(*ast.ReturnStmt); !ok || len(ret.Results) != 0 {
+			return true
+		}
+		var errVar types.Object
+		if as, ok := ifs.Init.(*ast.AssignStmt); ok && len(as.Lhs) == 1 && len(as.Rhs) == 1 && isDecodeCall(info, as.Rhs[0]) {
+			if id, ok := as.Lhs[0].(*ast.Ident); ok {
+				errVar = info.ObjectOf(id)
+			}
+		}
+		var decodeCheck ast.Expr
+		var visit func(e ast.Expr)
+		visit = func(e ast.Expr) {
+			be, ok := unparen(e).(*ast.BinaryExpr)
+			if !ok {
+				return
+			}
+			switch be.Op {
+			case token.LOR:
+				visit(be.X)
+				visit(be.Y)
+			case token.NEQ:
+				if !isNilIdent(info, be.Y) {
+					return
+				}
+				if isDecodeCall(info, be.X) {
+					decodeCheck = be
+				} else if id, ok := unparen(be.X).(*ast.Ident); ok && errVar != nil && info.ObjectOf(id) == errVar {
+					decodeCheck = be
+				}
+			}
+		}
+		visit(ifs.Cond)
+		if decodeCheck != nil {
+			report(pass, decodeCheck, EP004, "decode error dropped with a bare return; log it or pass it to an error handler, or a payload the code didn't expect vanishes without a trace")
+		}
+		return true
+	})
+}
+
+func isNilIdent(info *types.Info, e ast.Expr) bool {
+	id, ok := unparen(e).(*ast.Ident)
+	if !ok {
+		return false
+	}
+	_, isNil := info.Uses[id].(*types.Nil)
+	return isNil
+}