Feat: Add confusing-epoch rule #1556

Acollie · 2025-11-07T12:02:22Z

Summary

This PR introduces a new linting rule confusing-epoch that enforces clear naming conventions for variables storing epoch time values. Variables initialized with epoch time methods (Unix(), UnixMilli(), UnixMicro(), UnixNano()) must have names that indicate their time unit to prevent confusion and potential
bugs.

Motivation

When working with epoch timestamps, it's easy to mix up time units (seconds vs milliseconds vs nanoseconds), leading to subtle bugs. For example:

timestamp := time.Now().Unix()      // Is this seconds, milliseconds, or nanoseconds?
expiry := time.Now().UnixMilli()    // Unit is unclear from the name

This rule enforces that variable names clearly indicate the unit:

  timestampSec := time.Now().Unix()     // ✅ Clear it's seconds
  expiryMs := time.Now().UnixMilli()    // ✅ Clear it's milliseconds

Changes Made

New rule: rule/confusing_epoch.go - Implements the linting logic with support for:
- Unix() → requires suffix: "Sec", "Second", or "Seconds"
- UnixMilli() → requires suffix: "Milli" or "Ms"
- UnixMicro() → requires suffix: "Micro", "Microsecond", "Microseconds", or "Us"
- UnixNano() → requires suffix: "Nano" or "Ns"
Comprehensive tests: testdata/confusing_epoch.go covers:
- All four epoch methods
- Case-insensitive suffix matching
- Multiple variable declarations
- Regular assignments vs short declarations
- Edge cases (blank identifiers, struct fields, compound assignments)
Documentation:
- Added rule to README.md with description
- Added detailed documentation to RULES_DESCRIPTIONS.md with examples

…poch time variables

Copilot

Pull Request Overview

This PR introduces a new linting rule confusing-epoch to detect confusingly named variables when working with epoch time values. The rule enforces that variables initialized with time methods like Unix(), UnixMilli(), UnixMicro(), and UnixNano() should have names that clearly indicate their time unit (e.g., ending with "Sec", "Ms", "Nano", etc.) to prevent confusion and bugs when working with different time scales.

Key changes:

New rule implementation that validates variable naming for epoch time assignments
Comprehensive test data covering various edge cases (mixed case, suffix positioning, different time units)
Documentation and configuration updates to register the rule

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
rule/confusing_epoch.go	Implements the new ConfusingEpochRule with AST walking logic to check variable names initialized with epoch time methods
testdata/confusing_epoch.go	Comprehensive test data with expected MATCH patterns covering valid/invalid naming scenarios
test/confusing_epoch_test.go	Test harness following the standard pattern used by other rules
config/config.go	Registers the new rule in the allRules list
RULES_DESCRIPTIONS.md	Documents the rule with description, examples, and configuration details
README.md	Adds the rule to the main rules table

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

rule/confusing_epoch.go

Copilot · 2025-11-07T16:21:05Z

rule/confusing_epoch.go

+	return "confusing-epoch"
+}
+
+type confusingEpoch struct {


The type name confusingEpoch doesn't follow the naming convention used by other rules in this codebase. Other walker types are prefixed with lint (e.g., lintTimeNames, lintTimeEqual, lintConfusingNames). This type should be renamed to lintConfusingEpoch for consistency.

I am not tied to this name but it seemed logical to me.

rule/confusing_epoch.go

RULES_DESCRIPTIONS.md

alexandear · 2025-11-08T17:42:08Z

config/config.go

 	&rule.InefficientMapLookupRule{},
 	&rule.ForbiddenCallInWgGoRule{},
 	&rule.UnnecessaryIfRule{},
+	&rule.ConfusingEpochRule{},


Since this rule enforces naming for the vars, I propose to rename it to epoch-naming for consistency with error-naming and var-naming.

alexandear · 2025-11-08T17:42:43Z

rule/confusing_epoch.go

+		w.onFailure(lint.Failure{
+			Confidence: 0.9,
+			Node:       name,
+			Category:   lint.FailureCategoryTime,


Suggested change

Category: lint.FailureCategoryTime,

Category: lint.FailureCategoryNaming,

alexandear · 2025-11-08T17:46:18Z

testdata/confusing_epoch.go

+import "time"
+
+var (
+	creation        = time.Now().Unix() // MATCH /variable 'creation' initialized with Unix() should have a name end with one of [Sec Second Seconds]/


Could we simplify?

Suggested change

creation = time.Now().Unix() // MATCH /variable 'creation' initialized with Unix() should have a name end with one of [Sec Second Seconds]/

creation = time.Now().Unix() // MATCH /var creation should have a suffix Sec, Second, Seconds/

Similar rules:

revive/testdata/golint/var_naming.go

Lines 15 to 50 in 7a40133

var safeUrl = "HttPS://..iaMHost..Test:443/paTh^A%ef//./%41PaTH/..//?" // MATCH /var safeUrl should be safeURL/

var var_name int // MATCH /don't use underscores in Go names; var var_name should be varName/

type t_wow struct { // MATCH /don't use underscores in Go names; type t_wow should be tWow/

x_damn int // MATCH /don't use underscores in Go names; struct field x_damn should be xDamn/

Url *url.URL // MATCH /struct field Url should be URL/

}

const fooId = "blah" // MATCH /const fooId should be fooID/

func f_it() { // MATCH /don't use underscores in Go names; func f_it should be fIt/

more_underscore := 4 // MATCH /don't use underscores in Go names; var more_underscore should be moreUnderscore/

_ = more_underscore

var err error

if isEof := (err == io.EOF); isEof { // MATCH /var isEof should be isEOF/

more_underscore = 7 // should be okay

}

x := net_http.Request{} // should be okay

_ = x

var ips []net.IP

for _, theIp := range ips { // MATCH /range var theIp should be theIP/

_ = theIp

}

switch myJson := g(); { // MATCH /var myJson should be myJSON/

default:

_ = myJson

}

var y net_http.ResponseWriter // an interface

switch tApi := y.(type) { // MATCH /var tApi should be tAPI/

default:

_ = tApi

}

revive/testdata/golint/error_naming.go

Lines 11 to 37 in 7a40133

var unexp = errors.New("some unexported error") // MATCH /error var unexp should have name of the form errFoo/

// Exp ...

var Exp = errors.New("some exported error")

// MATCH:14 /error var Exp should have name of the form ErrFoo/

var (

e1 = fmt.Errorf("blah %d", 4) // MATCH /error var e1 should have name of the form errFoo/

// E2 ...

E2 = fmt.Errorf("blah %d", 5) // MATCH /error var E2 should have name of the form ErrFoo/

// check there is no false positive for blank identifier.

// This pattern that can be found in benchmarks and examples should be allowed.

// The fact that the error variable is not used is out of the scope of this rule.

_ = errors.New("ok")

)

func f() {

// the linter does not check local variables, this one is valid

var whatever = errors.New("ok")

_ = whatever

// same as above with a blank identifier

_ = errors.New("ok")

}

chavacava · 2025-11-09T17:20:45Z

rule/confusing_epoch.go

+		},
+	}
+
+	file.Pkg.TypeCheck()


Type check might fail, do we care?

chavacava · 2025-11-09T17:25:43Z

rule/confusing_epoch.go

+		for i, name := range v.Names {
+			// Skip if no initialization value
+			if i >= len(v.Values) {
+				continue


Why continue and not break ? My understanding is that if i >= len(v.Values) is true at iteration i it will be true in subsequent iterations.

chavacava · 2025-11-09T17:27:00Z

rule/confusing_epoch.go

+		// Handle var declarations
+		for i, name := range v.Names {
+			// Skip if no initialization value
+			if i >= len(v.Values) {


We do not need to evaluate the expression len(v.Values) at each iteration of the loop, we can just evaluate ones before entering the loop

chavacava · 2025-11-09T17:27:39Z

rule/confusing_epoch.go

+			if i >= len(v.Values) {
+				continue
+			}
+			w.check(name, v.Values[i])


Suggested change

w.check(name, v.Values[i])

w.check(name, v.Values[i])

chavacava · 2025-11-09T17:27:51Z

rule/confusing_epoch.go

+		if v.Tok != token.DEFINE && v.Tok != token.ASSIGN {
+			return w
+		}
+		for i, lhs := range v.Lhs {


Suggested change

for i, lhs := range v.Lhs {

for i, lhs := range v.Lhs {

chavacava · 2025-11-09T17:28:38Z

rule/confusing_epoch.go

+		}
+		for i, lhs := range v.Lhs {
+			if i >= len(v.Rhs) {
+				continue


chavacava · 2025-11-09T17:29:56Z

rule/confusing_epoch.go

+			return w
+		}
+		for i, lhs := range v.Lhs {
+			if i >= len(v.Rhs) {


The evaluation of len(v.Rhs) will result in the same value for every iteration of the loop, we can move this expression out of the loop

chavacava · 2025-11-09T17:33:02Z

rule/confusing_epoch.go

+			if ident, ok := lhs.(*ast.Ident); ok {
+				// Skip blank identifier
+				if ident.Name == "_" {
+					continue
+				}
+				w.check(ident, v.Rhs[i])
+			}


Suggested change

if ident, ok := lhs.(*ast.Ident); ok {

// Skip blank identifier

if ident.Name == "_" {

continue

}

w.check(ident, v.Rhs[i])

}

if ident, ok := lhs.(*ast.Ident); !ok || ident.Name == "_" {

continue

}

w.check(ident, v.Rhs[i])

chavacava · 2025-11-09T17:36:51Z

rule/confusing_epoch.go

+	if !ok {
+		return false
+	}
+	return named.Obj().Pkg() != nil && named.Obj().Pkg().Path() == "time" && named.Obj().Name() == "Time"


Suggested change

return named.Obj().Pkg() != nil && named.Obj().Pkg().Path() == "time" && named.Obj().Name() == "Time"

obj := named.Obj()

pkg := obj.Pkg()

return pkg != nil && pkg.Path() == "time" && obj.Name() == "Time"

Acollie added 2 commits November 7, 2025 11:53

feature: add confusing-epoch rule to enforce naming conventions for e…

ec881a7

…poch time variables

FIX: lint in Markdown

fdb80c0

alexandear requested review from alexandear, chavacava and Copilot and removed request for chavacava November 7, 2025 16:16

Copilot AI reviewed Nov 7, 2025

View reviewed changes

FIX: fix desc

7a40133

alexandear requested changes Nov 8, 2025

View reviewed changes

chavacava reviewed Nov 9, 2025

View reviewed changes

	Category: lint.FailureCategoryTime,
	Category: lint.FailureCategoryNaming,

	creation = time.Now().Unix() // MATCH /variable 'creation' initialized with Unix() should have a name end with one of [Sec Second Seconds]/
	creation = time.Now().Unix() // MATCH /var creation should have a suffix Sec, Second, Seconds/

	var safeUrl = "HttPS://..iaMHost..Test:443/paTh^A%ef//./%41PaTH/..//?" // MATCH /var safeUrl should be safeURL/
	var var_name int // MATCH /don't use underscores in Go names; var var_name should be varName/

	type t_wow struct { // MATCH /don't use underscores in Go names; type t_wow should be tWow/
	x_damn int // MATCH /don't use underscores in Go names; struct field x_damn should be xDamn/
	Url *url.URL // MATCH /struct field Url should be URL/
	}

	const fooId = "blah" // MATCH /const fooId should be fooID/

	func f_it() { // MATCH /don't use underscores in Go names; func f_it should be fIt/
	more_underscore := 4 // MATCH /don't use underscores in Go names; var more_underscore should be moreUnderscore/
	_ = more_underscore
	var err error
	if isEof := (err == io.EOF); isEof { // MATCH /var isEof should be isEOF/
	more_underscore = 7 // should be okay
	}

	x := net_http.Request{} // should be okay
	_ = x

	var ips []net.IP
	for _, theIp := range ips { // MATCH /range var theIp should be theIP/
	_ = theIp
	}

	switch myJson := g(); { // MATCH /var myJson should be myJSON/
	default:
	_ = myJson
	}
	var y net_http.ResponseWriter // an interface
	switch tApi := y.(type) { // MATCH /var tApi should be tAPI/
	default:
	_ = tApi
	}

	var unexp = errors.New("some unexported error") // MATCH /error var unexp should have name of the form errFoo/

	// Exp ...
	var Exp = errors.New("some exported error")

	// MATCH:14 /error var Exp should have name of the form ErrFoo/

	var (
	e1 = fmt.Errorf("blah %d", 4) // MATCH /error var e1 should have name of the form errFoo/
	// E2 ...
	E2 = fmt.Errorf("blah %d", 5) // MATCH /error var E2 should have name of the form ErrFoo/

	// check there is no false positive for blank identifier.
	// This pattern that can be found in benchmarks and examples should be allowed.
	// The fact that the error variable is not used is out of the scope of this rule.
	_ = errors.New("ok")
	)

	func f() {

	// the linter does not check local variables, this one is valid
	var whatever = errors.New("ok")
	_ = whatever

	// same as above with a blank identifier
	_ = errors.New("ok")
	}

-	return named.Obj().Pkg() != nil && named.Obj().Pkg().Path() == "time" && named.Obj().Name() == "Time"
+    obj := named.Obj()
+    pkg := obj.Pkg()
+	return pkg != nil && pkg.Path() == "time" && obj.Name() == "Time"

Feat: Add confusing-epoch rule #1556

Are you sure you want to change the base?

Feat: Add confusing-epoch rule #1556

Uh oh!

Conversation

Acollie commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Motivation

Changes Made

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Acollie commented Nov 7, 2025 •

edited

Loading