Change styleCheck to ignore foreign packages (nim-lang#19822)

* Change `styleCheck` to ignore foreign packages

* Symbols from foreign packages are now ignored.
* Fixed `styleCheck` violations in `compiler` package.
* Added symbol ownership to custom annotation pragmas.
* Minor refactors to cleanup style check callsites.
* Minor internal documentation of reasons why a symbol isn't checked.

Style violations were fixed in the compiler after thet were exposed by
the changes. The compiler wouldn't compile otherwise.

Symbol ownership for custom pragma annotations is needed for checking
the annotation's style. A NPE was raised otherwise.

Fixes nim-lang#10201
See also nim-lang/RFCs#456

* Fix a misunderstanding about excluding field style checks

I had refactored the callsites of `styleCheckUse` to apply the DRY
principle, but I misunderstood the field access handling in a template
as a general case. This corrects it.

* Fix some `styleCheck` violations in `compiler/evalffi`

The violations were exposed in CI when the compiler was built with
libffi.

* Removed some uneeded transitionary code

* Add changelog entry

Co-authored-by: quantimnot <quantimnot@users.noreply.github.com>

changelog.md

@@ -114,6 +114,8 @@ becomes an alias for `addr`.
 - `nim` can now compile version 1.4.0 as follows: `nim c --lib:lib --stylecheck:off compiler/nim`,
   without requiring `-d:nimVersion140` which is now a noop.
 
+- `--styleCheck` now only applies to the current package.
+
 
 ## Tool changes
 

compiler/evalffi.nim

@@ -110,8 +110,8 @@ proc mapCallConv(conf: ConfigRef, cc: TCallingConvention, info: TLineInfo): TABI
   else:
     globalError(conf, info, "cannot map calling convention to FFI")
 
-template rd(T, p: untyped): untyped = (cast[ptr T](p))[]
-template wr(T, p, v: untyped): untyped = (cast[ptr T](p))[] = v
+template rd(typ, p: untyped): untyped = (cast[ptr typ](p))[]
+template wr(typ, p, v: untyped): untyped = (cast[ptr typ](p))[] = v
 template `+!`(x, y: untyped): untyped =
   cast[pointer](cast[ByteAddress](x) + y)
 
@@ -177,8 +177,8 @@ const maxPackDepth = 20
 var packRecCheck = 0
 
 proc pack(conf: ConfigRef, v: PNode, typ: PType, res: pointer) =
-  template awr(T, v: untyped): untyped =
-    wr(T, res, v)
+  template awr(typ, v: untyped): untyped =
+    wr(typ, res, v)
 
   case typ.kind
   of tyBool: awr(bool, v.intVal != 0)

compiler/ic/bitabs.nim

@@ -93,13 +93,13 @@ proc getOrIncl*[T](t: var BiTable[T]; v: T): LitId =
   t.vals.add v
 
 
-proc `[]`*[T](t: var BiTable[T]; LitId: LitId): var T {.inline.} =
-  let idx = idToIdx LitId
+proc `[]`*[T](t: var BiTable[T]; litId: LitId): var T {.inline.} =
+  let idx = idToIdx litId
   assert idx < t.vals.len
   result = t.vals[idx]
 
-proc `[]`*[T](t: BiTable[T]; LitId: LitId): lent T {.inline.} =
-  let idx = idToIdx LitId
+proc `[]`*[T](t: BiTable[T]; litId: LitId): lent T {.inline.} =
+  let idx = idToIdx litId
   assert idx < t.vals.len
   result = t.vals[idx]
 

compiler/linter.nim

@@ -12,7 +12,8 @@
 import std/strutils
 from std/sugar import dup
 
-import options, ast, msgs, idents, lineinfos, wordrecg, astmsgs
+import options, ast, msgs, idents, lineinfos, wordrecg, astmsgs, semdata, packages
+export packages
 
 const
   Letters* = {'a'..'z', 'A'..'Z', '0'..'9', '\x80'..'\xFF', '_'}
@@ -85,24 +86,32 @@ proc differ*(line: string, a, b: int, x: string): string =
       result = y
 
 proc nep1CheckDefImpl(conf: ConfigRef; info: TLineInfo; s: PSym; k: TSymKind) =
-  # operators stay as they are:
-  if k in {skResult, skTemp} or s.name.s[0] notin Letters: return
-  if k in {skType, skGenericParam} and sfAnon in s.flags: return
-  if s.typ != nil and s.typ.kind == tyTypeDesc: return
-  if {sfImportc, sfExportc} * s.flags != {}: return
-  if optStyleCheck notin s.options: return
   let beau = beautifyName(s.name.s, k)
   if s.name.s != beau:
     lintReport(conf, info, beau, s.name.s)
 
-template styleCheckDef*(conf: ConfigRef; info: TLineInfo; s: PSym; k: TSymKind) =
-  if {optStyleHint, optStyleError} * conf.globalOptions != {} and optStyleUsages notin conf.globalOptions:
-    nep1CheckDefImpl(conf, info, s, k)
-
-template styleCheckDef*(conf: ConfigRef; info: TLineInfo; s: PSym) =
-  styleCheckDef(conf, info, s, s.kind)
-template styleCheckDef*(conf: ConfigRef; s: PSym) =
-  styleCheckDef(conf, s.info, s, s.kind)
+template styleCheckDef*(ctx: PContext; info: TLineInfo; sym: PSym; k: TSymKind) =
+  ## Check symbol definitions adhere to NEP1 style rules.
+  if optStyleCheck in ctx.config.options and # ignore if styleChecks are off
+     hintName in ctx.config.notes and # ignore if name checks are not requested
+     ctx.config.belongsToProjectPackage(ctx.module) and # ignore foreign packages
+     optStyleUsages notin ctx.config.globalOptions and # ignore if requested to only check name usage
+     sym.kind != skResult and # ignore `result`
+     sym.kind != skTemp and # ignore temporary variables created by the compiler
+     sym.name.s[0] in Letters and # ignore operators TODO: what about unicode symbols???
+     k notin {skType, skGenericParam} and # ignore types and generic params
+     (sym.typ == nil or sym.typ.kind != tyTypeDesc) and # ignore `typedesc`
+     {sfImportc, sfExportc} * sym.flags == {} and # ignore FFI
+     sfAnon notin sym.flags: # ignore if created by compiler
+    nep1CheckDefImpl(ctx.config, info, sym, k)
+
+template styleCheckDef*(ctx: PContext; info: TLineInfo; s: PSym) =
+  ## Check symbol definitions adhere to NEP1 style rules.
+  styleCheckDef(ctx, info, s, s.kind)
+
+template styleCheckDef*(ctx: PContext; s: PSym) =
+  ## Check symbol definitions adhere to NEP1 style rules.
+  styleCheckDef(ctx, s.info, s, s.kind)
 
 proc differs(conf: ConfigRef; info: TLineInfo; newName: string): string =
   let line = sourceLine(conf, info)
@@ -116,23 +125,27 @@ proc differs(conf: ConfigRef; info: TLineInfo; newName: string): string =
   let last = first+identLen(line, first)-1
   result = differ(line, first, last, newName)
 
-proc styleCheckUse*(conf: ConfigRef; info: TLineInfo; s: PSym) =
-  if info.fileIndex.int < 0: return
-  # we simply convert it to what it looks like in the definition
-  # for consistency
-
-  # operators stay as they are:
-  if s.kind == skTemp or s.name.s[0] notin Letters or sfAnon in s.flags:
-    return
-
+proc styleCheckUseImpl(conf: ConfigRef; info: TLineInfo; s: PSym) =
   let newName = s.name.s
   let badName = differs(conf, info, newName)
   if badName.len > 0:
-    # special rules for historical reasons
-    let forceHint = badName == "nnkArgList" and newName == "nnkArglist" or badName == "nnkArglist" and newName == "nnkArgList"
-    lintReport(conf, info, newName, badName, forceHint = forceHint, extraMsg = "".dup(addDeclaredLoc(conf, s)))
-
-proc checkPragmaUse*(conf: ConfigRef; info: TLineInfo; w: TSpecialWord; pragmaName: string) =
+    lintReport(conf, info, newName, badName, "".dup(addDeclaredLoc(conf, s)))
+
+template styleCheckUse*(ctx: PContext; info: TLineInfo; sym: PSym) =
+  ## Check symbol uses match their definition's style.
+  if {optStyleHint, optStyleError} * ctx.config.globalOptions != {} and # ignore if styleChecks are off
+     hintName in ctx.config.notes and # ignore if name checks are not requested
+     ctx.config.belongsToProjectPackage(ctx.module) and # ignore foreign packages
+     sym.kind != skTemp and # ignore temporary variables created by the compiler
+     sym.name.s[0] in Letters and # ignore operators TODO: what about unicode symbols???
+     sfAnon notin sym.flags: # ignore temporary variables created by the compiler
+    styleCheckUseImpl(ctx.config, info, sym)
+
+proc checkPragmaUseImpl(conf: ConfigRef; info: TLineInfo; w: TSpecialWord; pragmaName: string) =
   let wanted = $w
   if pragmaName != wanted:
     lintReport(conf, info, wanted, pragmaName)
+
+template checkPragmaUse*(conf: ConfigRef; info: TLineInfo; w: TSpecialWord; pragmaName: string) =
+  if {optStyleHint, optStyleError} * conf.globalOptions != {}:
+    checkPragmaUseImpl(conf, info, w, pragmaName)

compiler/msgs.nim

@@ -622,9 +622,9 @@ template internalAssert*(conf: ConfigRef, e: bool) =
     let arg = info2.toFileLineCol
     internalErrorImpl(conf, unknownLineInfo, arg, info2)
 
-template lintReport*(conf: ConfigRef; info: TLineInfo, beau, got: string, forceHint = false, extraMsg = "") =
+template lintReport*(conf: ConfigRef; info: TLineInfo, beau, got: string, extraMsg = "") =
   let m = "'$1' should be: '$2'$3" % [got, beau, extraMsg]
-  let msg = if optStyleError in conf.globalOptions and not forceHint: errGenerated else: hintName
+  let msg = if optStyleError in conf.globalOptions: errGenerated else: hintName
   liMessage(conf, info, msg, m, doNothing, instLoc())
 
 proc quotedFilename*(conf: ConfigRef; i: TLineInfo): Rope =

compiler/pragmas.nim

@@ -829,8 +829,7 @@ proc singlePragma(c: PContext, sym: PSym, n: PNode, i: var int,
   let ident = considerQuotedIdent(c, key)
   var userPragma = strTableGet(c.userPragmas, ident)
   if userPragma != nil:
-    if {optStyleHint, optStyleError} * c.config.globalOptions != {}:
-      styleCheckUse(c.config, key.info, userPragma)
+    styleCheckUse(c, key.info, userPragma)
 
     # number of pragmas increase/decrease with user pragma expansion
     inc c.instCounter
@@ -844,8 +843,7 @@ proc singlePragma(c: PContext, sym: PSym, n: PNode, i: var int,
   else:
     let k = whichKeyword(ident)
     if k in validPragmas:
-      if {optStyleHint, optStyleError} * c.config.globalOptions != {}:
-        checkPragmaUse(c.config, key.info, k, ident.s)
+      checkPragmaUse(c.config, key.info, k, ident.s)
       case k
       of wExportc, wExportCpp:
         makeExternExport(c, sym, getOptionalStr(c, it, "$1"), it.info)

compiler/semexprs.nim

@@ -2599,7 +2599,7 @@ proc semBlock(c: PContext, n: PNode; flags: TExprFlags): PNode =
       labl.owner = c.p.owner
     n[0] = newSymNode(labl, n[0].info)
     suggestSym(c.graph, n[0].info, labl, c.graph.usageSym)
-    styleCheckDef(c.config, labl)
+    styleCheckDef(c, labl)
     onDef(n[0].info, labl)
   n[1] = semExpr(c, n[1], flags)
   n.typ = n[1].typ

compiler/semgnrc.nim

@@ -179,7 +179,7 @@ proc fuzzyLookup(c: PContext, n: PNode, flags: TSemGenericFlags,
 proc addTempDecl(c: PContext; n: PNode; kind: TSymKind) =
   let s = newSymS(skUnknown, getIdentNode(c, n), c)
   addPrelimDecl(c, s)
-  styleCheckDef(c.config, n.info, s, kind)
+  styleCheckDef(c, n.info, s, kind)
   onDef(n.info, s)
 
 proc semGenericStmt(c: PContext, n: PNode,

compiler/semmagic.nim

@@ -428,7 +428,7 @@ proc semQuantifier(c: PContext; n: PNode): PNode =
       let op = considerQuotedIdent(c, it[0])
       if op.id == ord(wIn):
         let v = newSymS(skForVar, it[1], c)
-        styleCheckDef(c.config, v)
+        styleCheckDef(c, v)
         onDef(it[1].info, v)
         let domain = semExprWithType(c, it[2], {efWantIterator})
         v.typ = domain.typ

compiler/semstmts.nim

@@ -375,7 +375,7 @@ proc semUsing(c: PContext; n: PNode): PNode =
       let typ = semTypeNode(c, a[^2], nil)
       for j in 0..<a.len-2:
         let v = semIdentDef(c, a[j], skParam)
-        styleCheckDef(c.config, v)
+        styleCheckDef(c, v)
         onDef(a[j].info, v)
         v.typ = typ
         strTableIncl(c.signatures, v)
@@ -664,7 +664,7 @@ proc semVarOrLet(c: PContext, n: PNode, symkind: TSymKind): PNode =
         addToVarSection(c, result, n, a)
         continue
       var v = semIdentDef(c, a[j], symkind, false)
-      styleCheckDef(c.config, v)
+      styleCheckDef(c, v)
       onDef(a[j].info, v)
       if sfGenSym notin v.flags:
         if not isDiscardUnderscore(v): addInterfaceDecl(c, v)
@@ -794,7 +794,7 @@ proc semConst(c: PContext, n: PNode): PNode =
       var v = semIdentDef(c, a[j], skConst)
       if sfGenSym notin v.flags: addInterfaceDecl(c, v)
       elif v.owner == nil: v.owner = getCurrOwner(c)
-      styleCheckDef(c.config, v)
+      styleCheckDef(c, v)
       onDef(a[j].info, v)
 
       if a.kind != nkVarTuple:
@@ -819,7 +819,7 @@ include semfields
 proc symForVar(c: PContext, n: PNode): PSym =
   let m = if n.kind == nkPragmaExpr: n[0] else: n
   result = newSymG(skForVar, m, c)
-  styleCheckDef(c.config, result)
+  styleCheckDef(c, result)
   onDef(n.info, result)
   if n.kind == nkPragmaExpr:
     pragma(c, result, n[1], forVarPragmas)
@@ -1445,7 +1445,7 @@ proc typeSectionFinalPass(c: PContext, n: PNode) =
     let name = typeSectionTypeName(c, a[0])
     var s = name.sym
     # check the style here after the pragmas have been processed:
-    styleCheckDef(c.config, s)
+    styleCheckDef(c, s)
     # compute the type's size and check for illegal recursions:
     if a[1].kind == nkEmpty:
       var x = a[2]
@@ -2043,7 +2043,7 @@ proc semProcAux(c: PContext, n: PNode, kind: TSymKind,
       ("'" & proto.name.s & "' from " & c.config$proto.info &
         " '" & s.name.s & "' from " & c.config$s.info))
 
-  styleCheckDef(c.config, s)
+  styleCheckDef(c, s)
   if hasProto:
     onDefResolveForward(n[namePos].info, proto)
   else:
@@ -2120,7 +2120,7 @@ proc semProcAux(c: PContext, n: PNode, kind: TSymKind,
         trackProc(c, s, s.ast[bodyPos])
       else:
         if (s.typ[0] != nil and s.kind != skIterator):
-          addDecl(c, newSym(skUnknown, getIdent(c.cache, "result"), nextSymId c.idgen, nil, n.info))
+          addDecl(c, newSym(skUnknown, getIdent(c.cache, "result"), nextSymId c.idgen, s, n.info))
 
         openScope(c)
         n[bodyPos] = semGenericStmt(c, n[bodyPos])

compiler/semtempl.nim

@@ -230,7 +230,7 @@ proc addLocalDecl(c: var TemplCtx, n: var PNode, k: TSymKind) =
       if n.kind != nkSym:
         let local = newGenSym(k, ident, c)
         addPrelimDecl(c.c, local)
-        styleCheckDef(c.c.config, n.info, local)
+        styleCheckDef(c.c, n.info, local)
         onDef(n.info, local)
         replaceIdentBySym(c.c, n, newSymNode(local, n.info))
         if k == skParam and c.inTemplateHeader > 0:
@@ -271,8 +271,8 @@ proc semTemplSymbol(c: PContext, n: PNode, s: PSym; isField: bool): PNode =
     when defined(nimsuggest):
       suggestSym(c.graph, n.info, s, c.graph.usageSym, false)
     # field access (dot expr) will be handled by builtinFieldAccess
-    if not isField and {optStyleHint, optStyleError} * c.config.globalOptions != {}:
-      styleCheckUse(c.config, n.info, s)
+    if not isField:
+      styleCheckUse(c, n.info, s)
 
 proc semRoutineInTemplName(c: var TemplCtx, n: PNode): PNode =
   result = n
@@ -297,7 +297,7 @@ proc semRoutineInTemplBody(c: var TemplCtx, n: PNode, k: TSymKind): PNode =
       var s = newGenSym(k, ident, c)
       s.ast = n
       addPrelimDecl(c.c, s)
-      styleCheckDef(c.c.config, n.info, s)
+      styleCheckDef(c.c, n.info, s)
       onDef(n.info, s)
       n[namePos] = newSymNode(s, n[namePos].info)
     else:
@@ -431,7 +431,7 @@ proc semTemplBody(c: var TemplCtx, n: PNode): PNode =
         # labels are always 'gensym'ed:
         let s = newGenSym(skLabel, n[0], c)
         addPrelimDecl(c.c, s)
-        styleCheckDef(c.c.config, s)
+        styleCheckDef(c.c, s)
         onDef(n[0].info, s)
         n[0] = newSymNode(s, n[0].info)
     n[1] = semTemplBody(c, n[1])
@@ -624,7 +624,7 @@ proc semTemplateDef(c: PContext, n: PNode): PNode =
        s.owner.name.s == "vm" and s.name.s == "stackTrace":
       incl(s.flags, sfCallsite)
 
-  styleCheckDef(c.config, s)
+  styleCheckDef(c, s)
   onDef(n[namePos].info, s)
   # check parameter list:
   #s.scope = c.currentScope

compiler/semtypes.nim

@@ -139,7 +139,7 @@ proc semEnum(c: PContext, n: PNode, prev: PType): PType =
       e.flags.incl {sfUsed, sfExported}
 
     result.n.add symNode
-    styleCheckDef(c.config, e)
+    styleCheckDef(c, e)
     onDef(e.info, e)
     if sfGenSym notin e.flags:
       if not isPure:
@@ -476,7 +476,7 @@ proc semTuple(c: PContext, n: PNode, prev: PType): PType =
       else:
         result.n.add newSymNode(field)
         addSonSkipIntLit(result, typ, c.idgen)
-      styleCheckDef(c.config, a[j].info, field)
+      styleCheckDef(c, a[j].info, field)
       onDef(field.info, field)
   if result.n.len == 0: result.n = nil
   if isTupleRecursive(result):
@@ -808,7 +808,7 @@ proc semRecordNodeAux(c: PContext, n: PNode, check: var IntSet, pos: var int,
         localError(c.config, info, "attempt to redefine: '" & f.name.s & "'")
       if a.kind == nkEmpty: father.add newSymNode(f)
       else: a.add newSymNode(f)
-      styleCheckDef(c.config, f)
+      styleCheckDef(c, f)
       onDef(f.info, f)
     if a.kind != nkEmpty: father.add a
   of nkSym:
@@ -1315,7 +1315,7 @@ proc semProcTypeNode(c: PContext, n, genericParams: PNode,
       result.n.add newSymNode(arg)
       rawAddSon(result, finalType)
       addParamOrResult(c, arg, kind)
-      styleCheckDef(c.config, a[j].info, arg)
+      styleCheckDef(c, a[j].info, arg)
       onDef(a[j].info, arg)
       if {optNimV1Emulation, optNimV12Emulation} * c.config.globalOptions == {}:
         a[j] = newSymNode(arg)

compiler/suggest.nim

@@ -598,8 +598,7 @@ proc markUsed(c: PContext; info: TLineInfo; s: PSym) =
     if sfError in s.flags: userError(conf, info, s)
   when defined(nimsuggest):
     suggestSym(c.graph, info, s, c.graph.usageSym, false)
-  if {optStyleHint, optStyleError} * conf.globalOptions != {}:
-    styleCheckUse(conf, info, s)
+  styleCheckUse(c, info, s)
   markOwnerModuleAsUsed(c, s)
 
 proc safeSemExpr*(c: PContext, n: PNode): PNode =

compiler/vm.nim

@@ -119,13 +119,13 @@ template move(a, b: untyped) {.dirty.} = system.shallowCopy(a, b)
 
 proc derefPtrToReg(address: BiggestInt, typ: PType, r: var TFullReg, isAssign: bool): bool =
   # nim bug: `isAssign: static bool` doesn't work, giving odd compiler error
-  template fun(field, T, rkind) =
+  template fun(field, typ, rkind) =
     if isAssign:
-      cast[ptr T](address)[] = T(r.field)
+      cast[ptr typ](address)[] = typ(r.field)
     else:
       r.ensureKind(rkind)
-      let val = cast[ptr T](address)[]
-      when T is SomeInteger | char:
+      let val = cast[ptr typ](address)[]
+      when typ is SomeInteger | char:
         r.field = BiggestInt(val)
       else:
         r.field = val