Extended side effect error messages (#18418)
* Extended side effect error messages * Applied feedback: - refactored `markSideEffect` - refactored string interpolations - single message - skip diagnostics in `system.compiles` context Other: - started a test of diagnostic messages [ci skip] Tests aren't updated yet because messaging isn't nailed down. * - Added hints of where for side effect call locations. - Tried to clarify the reasons. * fix tests * Applied PR review feedback: - moved collection of side effects from TSym to TContext - used pragma shorthand form `.sideEffect` and `.noSideEffect` in messages - added leading '>' to structured messages for readability - changed `sempass2.markSideEffect` to a proc - replaced `system.echo` in the test to make the test compatible with Windows * Applied NEP1 formatting suggestion Co-authored-by: quantimnot <quantimnot@users.noreply.github.com>
This commit is contained in:
quantimnot
GitHub
parent
af3d2d8ad9
commit
a9701f6531
3 changed files with 95 additions and 17 deletions
|
|
@ -10,7 +10,7 @@
|
|||
import
|
||||
intsets, ast, astalgo, msgs, renderer, magicsys, types, idents, trees,
|
||||
wordrecg, strutils, options, guards, lineinfos, semfold, semdata,
|
||||
modulegraphs, varpartitions, typeallowed, nilcheck, errorhandling
|
||||
modulegraphs, varpartitions, typeallowed, nilcheck, errorhandling, tables
|
||||
|
||||
when defined(useDfa):
|
||||
import dfa
|
||||
|
|
@ -223,13 +223,21 @@ proc markGcUnsafe(a: PEffects; reason: PNode) =
|
|||
a.owner.gcUnsafetyReason = newSym(skUnknown, a.owner.name, nextSymId a.c.idgen,
|
||||
a.owner, reason.info, {})
|
||||
|
||||
when true:
|
||||
template markSideEffect(a: PEffects; reason: typed) =
|
||||
if not a.inEnforcedNoSideEffects: a.hasSideEffect = true
|
||||
else:
|
||||
template markSideEffect(a: PEffects; reason: typed) =
|
||||
if not a.inEnforcedNoSideEffects: a.hasSideEffect = true
|
||||
markGcUnsafe(a, reason)
|
||||
proc markSideEffect(a: PEffects; reason: PNode | PSym; useLoc: TLineInfo) =
|
||||
if not a.inEnforcedNoSideEffects:
|
||||
a.hasSideEffect = true
|
||||
if a.owner.kind in routineKinds:
|
||||
var sym: PSym
|
||||
when reason is PNode:
|
||||
if reason.kind == nkSym:
|
||||
sym = reason.sym
|
||||
else:
|
||||
let kind = if reason.kind == nkHiddenDeref: skParam else: skUnknown
|
||||
sym = newSym(kind, a.owner.name, nextSymId a.c.idgen, a.owner, reason.info, {})
|
||||
else:
|
||||
sym = reason
|
||||
a.c.sideEffects.mgetOrPut(a.owner.id, @[]).add (useLoc, sym)
|
||||
when false: markGcUnsafe(a, reason)
|
||||
|
||||
proc listGcUnsafety(s: PSym; onlyWarning: bool; cycleCheck: var IntSet; conf: ConfigRef) =
|
||||
let u = s.gcUnsafetyReason
|
||||
|
|
@ -259,6 +267,31 @@ proc listGcUnsafety(s: PSym; onlyWarning: bool; conf: ConfigRef) =
|
|||
var cycleCheck = initIntSet()
|
||||
listGcUnsafety(s, onlyWarning, cycleCheck, conf)
|
||||
|
||||
proc listSideEffects(result: var string; s: PSym; cycleCheck: var IntSet;
|
||||
conf: ConfigRef; context: PContext; indentLevel: int) =
|
||||
template addHint(msg; lineInfo; sym; level = indentLevel) =
|
||||
result.addf("$# $# Hint: '$#' $#\n", repeat(">", level), conf $ lineInfo, sym, msg)
|
||||
if context.sideEffects.hasKey(s.id):
|
||||
for (useLineInfo, u) in context.sideEffects[s.id]:
|
||||
if u != nil and not cycleCheck.containsOrIncl(u.id):
|
||||
case u.kind
|
||||
of skLet, skVar:
|
||||
addHint("accesses global state '$#'" % u.name.s, useLineInfo, s.name.s)
|
||||
addHint("accessed by '$#'" % s.name.s, u.info, u.name.s, indentLevel + 1)
|
||||
of routineKinds:
|
||||
addHint("calls `.sideEffect` '$#'" % u.name.s, useLineInfo, s.name.s)
|
||||
addHint("called by '$#'" % s.name.s, u.info, u.name.s, indentLevel + 1)
|
||||
listSideEffects(result, u, cycleCheck, conf, context, indentLevel + 2)
|
||||
of skParam, skForVar:
|
||||
addHint("calls routine via hidden pointer indirection", useLineInfo, s.name.s)
|
||||
else:
|
||||
addHint("calls routine via pointer indirection", useLineInfo, s.name.s)
|
||||
|
||||
proc listSideEffects(result: var string; s: PSym; conf: ConfigRef; context: PContext) =
|
||||
var cycleCheck = initIntSet()
|
||||
result.addf("'$#' can have side effects\n", s.name.s)
|
||||
listSideEffects(result, s, cycleCheck, conf, context, 1)
|
||||
|
||||
proc useVarNoInitCheck(a: PEffects; n: PNode; s: PSym) =
|
||||
if {sfGlobal, sfThread} * s.flags != {} and s.kind in {skVar, skLet} and
|
||||
s.magic != mNimvm:
|
||||
|
|
@ -267,9 +300,7 @@ proc useVarNoInitCheck(a: PEffects; n: PNode; s: PSym) =
|
|||
(tfHasGCedMem in s.typ.flags or s.typ.isGCedMem):
|
||||
#if a.config.hasWarn(warnGcUnsafe): warnAboutGcUnsafe(n)
|
||||
markGcUnsafe(a, s)
|
||||
markSideEffect(a, s)
|
||||
else:
|
||||
markSideEffect(a, s)
|
||||
markSideEffect(a, s, n.info)
|
||||
if s.owner != a.owner and s.kind in {skVar, skLet, skForVar, skResult, skParam} and
|
||||
{sfGlobal, sfThread} * s.flags == {}:
|
||||
a.isInnerProc = true
|
||||
|
|
@ -502,7 +533,7 @@ proc propagateEffects(tracked: PEffects, n: PNode, s: PSym) =
|
|||
if tracked.config.hasWarn(warnGcUnsafe): warnAboutGcUnsafe(n, tracked.config)
|
||||
markGcUnsafe(tracked, s)
|
||||
if tfNoSideEffect notin s.typ.flags:
|
||||
markSideEffect(tracked, s)
|
||||
markSideEffect(tracked, s, n.info)
|
||||
mergeLockLevels(tracked, n, s.getLockLevel)
|
||||
|
||||
proc procVarCheck(n: PNode; conf: ConfigRef) =
|
||||
|
|
@ -584,7 +615,7 @@ proc trackOperandForIndirectCall(tracked: PEffects, n: PNode, paramType: PType;
|
|||
if tracked.config.hasWarn(warnGcUnsafe): warnAboutGcUnsafe(n, tracked.config)
|
||||
markGcUnsafe(tracked, a)
|
||||
elif tfNoSideEffect notin op.flags and not isOwnedProcVar(a, tracked.owner):
|
||||
markSideEffect(tracked, a)
|
||||
markSideEffect(tracked, a, n.info)
|
||||
else:
|
||||
mergeRaises(tracked, effectList[exceptionEffects], n)
|
||||
mergeTags(tracked, effectList[tagEffects], n)
|
||||
|
|
@ -592,7 +623,7 @@ proc trackOperandForIndirectCall(tracked: PEffects, n: PNode, paramType: PType;
|
|||
if tracked.config.hasWarn(warnGcUnsafe): warnAboutGcUnsafe(n, tracked.config)
|
||||
markGcUnsafe(tracked, a)
|
||||
elif tfNoSideEffect notin op.flags:
|
||||
markSideEffect(tracked, a)
|
||||
markSideEffect(tracked, a, n.info)
|
||||
if paramType != nil and paramType.kind in {tyVar}:
|
||||
invalidateFacts(tracked.guards, n)
|
||||
if n.kind == nkSym and isLocalVar(tracked, n.sym):
|
||||
|
|
@ -749,7 +780,7 @@ proc trackCall(tracked: PEffects; n: PNode) =
|
|||
if tfNoSideEffect notin op.flags and not importedFromC(a):
|
||||
# and it's not a recursive call:
|
||||
if not (a.kind == nkSym and a.sym == tracked.owner):
|
||||
markSideEffect(tracked, a)
|
||||
markSideEffect(tracked, a, n.info)
|
||||
|
||||
# p's effects are ours too:
|
||||
var a = n[0]
|
||||
|
|
@ -771,7 +802,7 @@ proc trackCall(tracked: PEffects; n: PNode) =
|
|||
if a.sym == tracked.owner: tracked.isRecursive = true
|
||||
# even for recursive calls we need to check the lock levels (!):
|
||||
mergeLockLevels(tracked, n, a.sym.getLockLevel)
|
||||
if sfSideEffect in a.sym.flags: markSideEffect(tracked, a)
|
||||
if sfSideEffect in a.sym.flags: markSideEffect(tracked, a, n.info)
|
||||
else:
|
||||
mergeLockLevels(tracked, n, op.lockLevel)
|
||||
var effectList = op.n[0]
|
||||
|
|
@ -1336,6 +1367,7 @@ proc trackProc*(c: PContext; s: PSym, body: PNode) =
|
|||
effects[ensuresEffects] = ensuresSpec
|
||||
|
||||
var mutationInfo = MutationInfo()
|
||||
var hasMutationSideEffect = false
|
||||
if {strictFuncs, views} * c.features != {}:
|
||||
var goals: set[Goal] = {}
|
||||
if strictFuncs in c.features: goals.incl constParameters
|
||||
|
|
@ -1343,6 +1375,7 @@ proc trackProc*(c: PContext; s: PSym, body: PNode) =
|
|||
var partitions = computeGraphPartitions(s, body, g, goals)
|
||||
if not t.hasSideEffect and t.hasDangerousAssign:
|
||||
t.hasSideEffect = varpartitions.hasSideEffect(partitions, mutationInfo)
|
||||
hasMutationSideEffect = t.hasSideEffect
|
||||
if views in c.features:
|
||||
checkBorrowedLocations(partitions, body, g.config)
|
||||
|
||||
|
|
@ -1357,7 +1390,14 @@ proc trackProc*(c: PContext; s: PSym, body: PNode) =
|
|||
when false:
|
||||
listGcUnsafety(s, onlyWarning=false, g.config)
|
||||
else:
|
||||
localError(g.config, s.info, ("'$1' can have side effects" % s.name.s) & (g.config $ mutationInfo))
|
||||
if hasMutationSideEffect:
|
||||
localError(g.config, s.info, "'$1' can have side effects$2" % [s.name.s, g.config $ mutationInfo])
|
||||
elif c.compilesContextId == 0: # don't render extended diagnostic messages in `system.compiles` context
|
||||
var msg = ""
|
||||
listSideEffects(msg, s, g.config, t.c)
|
||||
message(g.config, s.info, errGenerated, msg)
|
||||
else:
|
||||
localError(g.config, s.info, "") # simple error for `system.compiles` context
|
||||
if not t.gcUnsafe:
|
||||
s.typ.flags.incl tfGcSafe
|
||||
if not t.hasSideEffect and sfSideEffect notin s.flags:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue