noliteral: Add ignoring of nested functions #17

achuprov · 2024-02-05T07:16:48Z

achuprov commented

2024-02-05 07:16:48 +00:00

Add ignoring of nested functions.
Example: ignore zap.Error(invalid_pkg.Error) in

func (c *cfg) debug1_ok() { 
	c.log.Debug(tochno_ne_dly_const_log.MSG, zap.Stringer("cid", bkt.CID), zap.String("oid", obj.VersionID), zap.Error(invalid_pkg.Error))
}

Close #16

Signed-off-by: Alexander Chuprov a.chuprov@yadro.com

Add ignoring of nested functions. Example: ignore ```zap.Error(invalid_pkg.Error)``` in ``` func (c *cfg) debug1_ok() { c.log.Debug(tochno_ne_dly_const_log.MSG, zap.Stringer("cid", bkt.CID), zap.String("oid", obj.VersionID), zap.Error(invalid_pkg.Error)) } ``` Close #16 Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>

achuprov requested review from dstepanov-yadro 2024-02-05 07:17:01 +00:00

achuprov force-pushed bugfix/noliteral_ignoring_nested_functions from dea5a71c18 to e1322ab51f

2024-02-05 23:08:12 +00:00

Compare

aarifullin reviewed 2024-02-06 08:06:59 +00:00

internal/analyzers/noliteral/linter.go Outdated

					
				@ -100,0 +91,4 @@

					*endFunctionCall = expr.End()

					if !astutils.IsStringValue(expr.Args[0]) {

						alias, _ := astutils.GetAliasByPkgName(file, Config.ConstantsPackage)

aarifullin commented

2024-02-06 08:06:59 +00:00

I still do not see any check for non-package usage like zap.Error(obj.Error)

Could this solve the problem? :)

func IsTypedValueSpec(expr ast.Expr) bool {
	selExp, ok := expr.(*ast.SelectorExpr)
	if !ok {
		return false
	}
	ident, ok := selExp.X.(*ast.Ident)
	if ok {
		if valExp, ok := ident.Obj.Decl.(*ast.ValueSpec); ok {
			if valExp.Type != nil {
				return true
			}
		}
	}
	return false
}

I still do not see any check for non-package usage like `zap.Error(obj.Error)` Could this solve the problem? :) ```go func IsTypedValueSpec(expr ast.Expr) bool { selExp, ok := expr.(*ast.SelectorExpr) if !ok { return false } ident, ok := selExp.X.(*ast.Ident) if ok { if valExp, ok := ident.Obj.Decl.(*ast.ValueSpec); ok { if valExp.Type != nil { return true } } } return false } ```

achuprov commented

2024-02-12 14:28:46 +00:00

We cannot rely on valExp.Type because it is only available when explicitly declared as var name Type. Example: dba7fe3257

We cannot rely on `valExp.Type` because it is only available when explicitly declared as `var name Type`. Example: https://astexplorer.net/#/gist/e53ef7889f8764c62cd8719f823b20c0/dba7fe32576e8272abdd466f6d7e00264266e87b

fyrchik reviewed 2024-02-06 08:30:03 +00:00

internal/analyzers/noliteral/linter.go Outdated

					
				@ -58,3 +59,2 @@

							isLog, _ := astutils.IsTargetMethod(expr.Fun, Config.TargetMethods)

							if !isLog || len(expr.Args) == 0 || astutils.HasNoLintComment(pass, expr.Pos()) {

							if n.Pos() < endPosAssignment {

fyrchik commented

2024-02-06 08:29:59 +00:00

Using Pos() checks feels wrong: we are traversing ast, why do we need to compare positions of bytes in text?

I looks the only thing that needed to be changed is IsTargetMethod: sel.X is the part before the dot and in our case it should be some object, not the package. Or is it not enough?

Using `Pos()` checks feels wrong: we are traversing ast, why do we need to compare positions of bytes in text? I looks the only thing that needed to be changed is `IsTargetMethod`: `sel.X` is the part before the dot and in our case it should be some object, not the package. Or is it not enough?

achuprov commented

2024-02-12 14:22:19 +00:00

You are right. We can choose not to use Pos().

We can't exclude all functions that are not part of an object. This is because there are packages with package-level logging, like https://github.com/sirupsen/logrus. My solution has a drawback: we can't distinguish between &zap.Error(...) and &log.Error(...), but this does not seem to be a common issue.

You are right. We can choose not to use `Pos()`. We can't exclude all functions that are not part of an object. This is because there are packages with package-level logging, like https://github.com/sirupsen/logrus. My solution has a drawback: we can't distinguish between `&zap.Error(...)` and `&log.Error(...)`, but this does not seem to be a common issue.