Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extended side effect error messages #18418

Merged
merged 11 commits into from
Jul 15, 2021
1 change: 1 addition & 0 deletions compiler/ast.nim
Original file line number Diff line number Diff line change
Expand Up @@ -857,6 +857,7 @@ type
of routineKinds:
#procInstCache*: seq[PInstantiation]
gcUnsafetyReason*: PSym # for better error messages wrt gcsafe
sideEffectReasons*: seq[PSym] # for better side effect error messages
transformedBody*: PNode # cached body after transf pass
of skLet, skVar, skField, skForVar:
guard*: PSym
Expand Down
2 changes: 2 additions & 0 deletions compiler/lineinfos.nim
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ type
warnCannotOpen = "CannotOpen",
warnFileChanged = "FileChanged",
warnUser = "User",
warnSideEffect = "SideEffect",
# hints
hintSuccess = "Success", hintSuccessX = "SuccessX",
hintCC = "CC",
Expand Down Expand Up @@ -150,6 +151,7 @@ const
warnCannotOpen: "cannot open: $1",
warnFileChanged: "file changed: $1",
warnUser: "$1",
warnSideEffect: "$1",
hintSuccess: "operation successful: $#",
# keep in sync with `testament.isSuccess`
hintSuccessX: "$build\n$loc lines; ${sec}s; $mem; proj: $project; out: $output",
Expand Down
46 changes: 44 additions & 2 deletions compiler/sempass2.nim
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,17 @@ proc markGcUnsafe(a: PEffects; reason: PNode) =

when true:
template markSideEffect(a: PEffects; reason: typed) =
if not a.inEnforcedNoSideEffects: a.hasSideEffect = true
if not a.inEnforcedNoSideEffects:
a.hasSideEffect = true
if a.owner.kind in routineKinds:
when reason is PNode:
if reason.kind == nkSym:
a.owner.sideEffectReasons.add reason.sym
else:
a.owner.sideEffectReasons.add newSym(skUnknown, a.owner.name, nextSymId a.c.idgen,
a.owner, reason.info, {})
else:
a.owner.sideEffectReasons.add reason
quantimnot marked this conversation as resolved.
Show resolved Hide resolved
else:
template markSideEffect(a: PEffects; reason: typed) =
quantimnot marked this conversation as resolved.
Show resolved Hide resolved
if not a.inEnforcedNoSideEffects: a.hasSideEffect = true
Expand Down Expand Up @@ -259,6 +269,33 @@ proc listGcUnsafety(s: PSym; onlyWarning: bool; conf: ConfigRef) =
var cycleCheck = initIntSet()
listGcUnsafety(s, onlyWarning, cycleCheck, conf)

proc listSideEffects(s: PSym; onlyWarning: bool; cycleCheck: var IntSet; conf: ConfigRef) =
for u in s.sideEffectReasons:
quantimnot marked this conversation as resolved.
Show resolved Hide resolved
if u != nil and not cycleCheck.containsOrIncl(u.id):
let msgKind = if onlyWarning: warnSideEffect else: errGenerated
case u.kind
of skLet, skVar:
message(conf, s.info, msgKind,
("'$#' accesses global state at '$#'") % [s.name.s, u.name.s])
of routineKinds:
# recursive call *always* produces only a warning so the full error
# message is printed:
listSideEffects(u, true, cycleCheck, conf)
message(conf, s.info, msgKind,
"'$#' has side effects as it calls '$#'" %
[s.name.s, u.name.s])
of skParam, skForVar:
message(conf, s.info, msgKind,
"'$#' has side effects as it performs an indirect call via '$#'" %
[s.name.s, u.name.s])
else:
message(conf, u.info, msgKind,
quantimnot marked this conversation as resolved.
Show resolved Hide resolved
"'$#' has side effects as it performs an indirect call here" % s.name.s)

proc listSideEffects(s: PSym; onlyWarning: bool; conf: ConfigRef) =
var cycleCheck = initIntSet()
listSideEffects(s, onlyWarning, cycleCheck, conf)

proc useVarNoInitCheck(a: PEffects; n: PNode; s: PSym) =
if {sfGlobal, sfThread} * s.flags != {} and s.kind in {skVar, skLet} and
s.magic != mNimvm:
Expand Down Expand Up @@ -1336,13 +1373,15 @@ 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
if views in c.features: goals.incl borrowChecking
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)

Expand All @@ -1357,7 +1396,10 @@ 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" % s.name.s) & (g.config $ mutationInfo))
quantimnot marked this conversation as resolved.
Show resolved Hide resolved
else:
listSideEffects(s, false, g.config)
if not t.gcUnsafe:
s.typ.flags.incl tfGcSafe
if not t.hasSideEffect and sfSideEffect notin s.flags:
Expand Down