From 9929164d96faba0dbb0a654effde4d27cd2a64e0 Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Thu, 9 Feb 2023 20:48:34 +0100 Subject: [PATCH] perform `.closure` inference in `sempass2` ## Summary While the `lambdalifting` transformation is responsible for figuring out what inner procedures capture the content of which memory locations, semantic analysis already needs to know whether an inner procedure accesses its environment, as whether it does affects its calling convention (which is part of a procedure's type). `semCaptureSym` was responsible for detecting if usage of a symbol required a capture and for changing all relevant enclosing procedures between the to use the `.closure` calling convention, but the implementation had multiple issues. Instead, whether a procedure captures something is now detected during `sempass2`. Setting the calling convention is performed there too now. This fixes: - inner procedures using the `.closure` calling convention even if explicitly specified otherwise - tentative semantic analysis (such as `compiles` or analysis of argument expression during overload resolution) having side-effects because of `semCaptureSym` - symbols resulting from typed template/macro parameter expansion weren't considered by `semCaptureSym`, which in some cases caused procedures to not be updated to use the `.closure` calling convention, which then caused errors in the code-generator or backend compiler Detecting illegal captures now also happens in `sempass2` (instead of in `lambdalifting`), which means that they're now *always* reported and not only for alive code. A hint is now reported if a procedure uses the `.closure` calling convention but doesn't capture anything. ## Details - move illegal capture detection from `lambdalifting` into `sempass2` - detect whether a procedure captures something during `sempass2` instead of in `lambdalifting` - remove `semCaptureSym` and its usages - remove the `tfCapturesEnv` type flag. It's use case was detecting top-level closure procedures (can only be achieved via anonymous procedures), but a hidden environment parameter is added for them now, instead - report a dedicated error when attempting to capture a run-time value in a compile-time context Changes to `lambdalifting`: - remove the unused "in container" analysis - remove all logic for permanently changing a procedure's calling convention - run the pass on all non-`.closure` procedures (including top-level closure iterators) instead of only on top-level procedures. While this doesn't affect the result (all inner procedures that capture something are still lifted), it means less recursion depth and a smaller working set for each run, as well as less calls to `transformBody` (which translates to less work). ### Misc - change `rsemIllegalMemoryCapture` to use the `sym` field instead of `symbols` - move `isOwnedBy` from `vmgen.nim` to `ast_query.nim` Co-authored-by: Saem Ghani --- compiler/ast/ast_query.nim | 8 + compiler/ast/ast_types.nim | 1 - compiler/ast/report_enums.nim | 3 +- compiler/backend/cgen.nim | 2 +- compiler/front/cli_reporter.nim | 11 +- compiler/plugins/itersgen.nim | 1 - compiler/sem/lambdalifting.nim | 163 +++++++--------- compiler/sem/sem.nim | 1 - compiler/sem/semexprs.nim | 5 - compiler/sem/sempass2.nim | 174 +++++++++++++++++- compiler/sem/semstmts.nim | 4 +- compiler/vm/vmaux.nim | 2 +- compiler/vm/vmgen.nim | 10 +- tests/lang_callable/closure/tclosure.nim | 6 + .../lang_callable/closure/tclosure_issues.nim | 35 ++++ .../closure/tillegal_comptime_capture.nim | 30 +++ .../lang_callable/closure/tlate_inference.nim | 87 +++++++++ 17 files changed, 414 insertions(+), 129 deletions(-) create mode 100644 tests/lang_callable/closure/tillegal_comptime_capture.nim create mode 100644 tests/lang_callable/closure/tlate_inference.nim diff --git a/compiler/ast/ast_query.nim b/compiler/ast/ast_query.nim index 70f552760b5..96d66336121 100644 --- a/compiler/ast/ast_query.nim +++ b/compiler/ast/ast_query.nim @@ -548,6 +548,14 @@ proc skipGenericOwner*(s: PSym): PSym = else: s.owner +func isOwnedBy*(a, b: PSym): bool = + ## Tests if `b` is the transitive owner of `a`, returns true if `a` got + ## owned! :) + var a = a.owner + while a != nil and a.kind != skModule: + if a == b: return true + a = a.owner + proc originatingModule*(s: PSym): PSym = result = s.owner while result.kind != skModule: result = result.owner diff --git a/compiler/ast/ast_types.nim b/compiler/ast/ast_types.nim index 14d86822c19..8d5b5a6c2f4 100644 --- a/compiler/ast/ast_types.nim +++ b/compiler/ast/ast_types.nim @@ -530,7 +530,6 @@ type ## concrete type (lastSon becomes the concrete type) tfRetType, ## marks return types in proc (used to detect type classes ## used as return types for return type inference) - tfCapturesEnv, ## whether proc really captures some environment tfByCopy, ## pass object/tuple by copy (C backend) tfByRef, ## pass object/tuple by reference (C backend) tfIterator, ## type is really an iterator, not a tyProc diff --git a/compiler/ast/report_enums.nim b/compiler/ast/report_enums.nim index d7ea6864812..a55ed7eabca 100644 --- a/compiler/ast/report_enums.nim +++ b/compiler/ast/report_enums.nim @@ -492,6 +492,7 @@ type rsemNotABaseMethod rsemIllegalCallconvCapture rsemIllegalMemoryCapture + rsemIllegalCompTimeCapture rsemIgnoreInvalidForLoop rsemMissingGenericParamsForTemplate rsemMisplacedMagicType @@ -842,6 +843,7 @@ type rsemXCannotRaiseY = "XCannotRaiseY" rsemConvToBaseNotNeeded = "ConvToBaseNotNeeded" rsemConvFromXtoItselfNotNeeded = "ConvFromXtoItselfNotNeeded" + rsemClosureWithoutEnv = "ClosureWithoutEnv" rsemProcessing = "Processing" ## Processing module rsemProcessingStmt = "ProcessingStmt" ## Processing toplevel statement @@ -1146,7 +1148,6 @@ const rsemUnexpectedPragmaInDefinitionOf, rsemDoubleCompletionOf, - rsemIllegalMemoryCapture, rsemOverrideSafetyMismatch, rsemOverrideLockMismatch } diff --git a/compiler/backend/cgen.nim b/compiler/backend/cgen.nim index 1d8eccf6962..02f31921cd3 100644 --- a/compiler/backend/cgen.nim +++ b/compiler/backend/cgen.nim @@ -859,7 +859,7 @@ proc closeNamespaceNim(): Rope = result.add("}\L") proc closureSetup(p: BProc, prc: PSym) = - if tfCapturesEnv notin prc.typ.flags: return + if prc.typ.callConv != ccClosure: return # prc.ast[paramsPos].last contains the type we're after: var ls = lastSon(prc.ast[paramsPos]) p.config.internalAssert(ls.kind == nkSym, prc.info, "closure generation failed") diff --git a/compiler/front/cli_reporter.nim b/compiler/front/cli_reporter.nim index 9090b2be517..ee867fdf015 100644 --- a/compiler/front/cli_reporter.nim +++ b/compiler/front/cli_reporter.nim @@ -458,12 +458,15 @@ proc reportBody*(conf: ConfigRef, r: SemReport): string = result = "duplicate case label" of rsemIllegalMemoryCapture: - let s = r.symbols[0] + let s = r.sym result = ( "'$1' is of type <$2> which cannot be captured as it would violate memory" & " safety, declared here: $3; using '-d:nimNoLentIterators' helps in some cases" ) % [s.name.s, typeToString(s.typ), conf $ s.info] - + of rsemIllegalCompTimeCapture: + result = "'$1' is inaccesible because run-time values cannot be" & + " captured in a compile-time context" + result = result % [r.sym.name.s] of rsemUnavailableTypeBound: result.add( "'", @@ -1414,7 +1417,9 @@ proc reportBody*(conf: ConfigRef, r: SemReport): string = of rsemConvFromXtoItselfNotNeeded: result = "conversion from $1 to itself is pointless" % r.typ.render - + of rsemClosureWithoutEnv: + result = "procedure explicitly uses the 'closure' calling convention," & + " but it doesn't capture anything" of rsemIllegalConversion: result = "illegal conversion from '$1' to '$2'" % [ r.actualType.render, r.formalType.render diff --git a/compiler/plugins/itersgen.nim b/compiler/plugins/itersgen.nim index f18a96f2600..6e54a867280 100644 --- a/compiler/plugins/itersgen.nim +++ b/compiler/plugins/itersgen.nim @@ -62,7 +62,6 @@ proc iterToProcImpl*(c: PContext, n: PNode): PNode = let prc = newSym(skProc, n[3].ident, nextSymId c.idgen, iter.sym.owner, iter.sym.info) prc.typ = copyType(iter.sym.typ, nextTypeId c.idgen, prc) - excl prc.typ.flags, tfCapturesEnv prc.typ.n.add newSymNode(getEnvParam(iter.sym)) prc.typ.rawAddSon t let orig = iter.sym.ast diff --git a/compiler/sem/lambdalifting.nim b/compiler/sem/lambdalifting.nim index 7646cc5a19f..54b34daa542 100644 --- a/compiler/sem/lambdalifting.nim +++ b/compiler/sem/lambdalifting.nim @@ -31,7 +31,6 @@ import msgs ], compiler/sem/[ - typeallowed, liftdestructors, transf, lowerings @@ -39,8 +38,7 @@ import # xxx: reports are a code smell meaning data types are misplaced from compiler/ast/reports_sem import SemReport, - reportSem, - reportSymbols + reportSem from compiler/ast/report_enums import ReportKind discard """ @@ -192,7 +190,6 @@ proc addHiddenParam(routine: PSym, param: PSym) = # some nkEffect node: param.position = routine.typ.n.len-1 params.add newSymNode(param) - #incl(routine.typ.flags, tfCapturesEnv) assert sfFromGeneric in param.flags #echo "produced environment: ", param.id, " for ", routine.id @@ -219,9 +216,6 @@ proc interestingVar(s: PSym): bool {.inline.} = sfGlobal notin s.flags and s.typ.kind notin {tyStatic, tyTypeDesc} -proc illegalCapture(s: PSym): bool {.inline.} = - result = classifyViewType(s.typ) != noView or s.kind == skResult - proc isInnerProc(s: PSym): bool = if s.kind in {skProc, skFunc, skMethod, skConverter, skIterator} and s.magic == mNone: result = s.skipGenericOwner.kind in routineKinds @@ -310,23 +304,6 @@ proc freshVarForClosureIter*(g: ModuleGraph; s: PSym; idgen: IdGenerator; owner: # ------------------ new stuff ------------------------------------------- -proc markAsClosure(g: ModuleGraph; owner: PSym; n: PNode) = - let s = n.sym - if illegalCapture(s): - localReport(g.config, n.info, reportSymbols( - rsemIllegalMemoryCapture, @[s, owner])) - - elif not ( - owner.typ.callConv == ccClosure or - owner.typ.callConv == ccNimCall and - tfExplicitCallConv notin owner.typ.flags - ): - localReport(g.config, n.info, reportSymbols( - rsemIllegalCallconvCapture, @[s, owner])) - - incl(owner.typ.flags, tfCapturesEnv) - owner.typ.callConv = ccClosure - type DetectionPass = object processed, capturedVars: IntSet @@ -428,6 +405,7 @@ Consider: """ proc addClosureParam(c: var DetectionPass; fn: PSym; info: TLineInfo) = + assert fn.typ.callConv == ccClosure var cp = getEnvParam(fn) let owner = if fn.kind == skIterator: fn else: fn.skipGenericOwner let t = c.getEnvTypeForOwner(owner, info) @@ -443,20 +421,24 @@ proc addClosureParam(c: var DetectionPass; fn: PSym; info: TLineInfo) = proc detectCapturedVars(n: PNode; owner: PSym; c: var DetectionPass) = case n.kind of nkSym: - let s = n.sym - if s.kind in {skProc, skFunc, skMethod, skConverter, skIterator} and - s.typ != nil and s.typ.callConv == ccClosure: - # this handles the case that the inner proc was declared as - # .closure but does not actually capture anything: - addClosureParam(c, s, n.info) + let + s = n.sym + innerProc = isInnerProc(s) + + if innerProc and s.typ.callConv == ccClosure: + # something that uses an environment parameter (even if it doesn't + # capture anything) is used that takes a closure is used -> the + # transformation pass is required c.somethingToDo = true - let innerProc = isInnerProc(s) - if innerProc: - if s.isIterator: c.somethingToDo = true if not c.processed.containsOrIncl(s.id): + # add the hidden parameter to it and process the inner procedure + addClosureParam(c, s, n.info) + # note that during ``transformBody``, the lambda lifting pass is run + # for the procedure let body = transformBody(c.graph, c.idgen, s, cache = true) detectCapturedVars(body, s, c) + let ow = s.skipGenericOwner if ow == owner: if owner.isIterator: @@ -473,26 +455,17 @@ proc detectCapturedVars(n: PNode; owner: PSym; c: var DetectionPass) = addField(obj, s, c.graph.cache, c.idgen) # direct or indirect dependency: elif (innerProc and s.typ.callConv == ccClosure) or interestingVar(s): - discard """ - proc outer() = - var x: int - proc inner() = - proc innerInner() = - echo x - innerInner() - inner() - # inner() takes a closure too! - """ - # mark 'owner' as taking a closure: + # an interesting entity that's not owned by the current `owner` (and + # thus needs to be captured in some form) c.somethingToDo = true - markAsClosure(c.graph, owner, n) addClosureParam(c, owner, n.info) - #echo "capturing ", n.info - # variable 's' is actually captured: + if interestingVar(s) and not c.capturedVars.containsOrIncl(s.id): + # we haven't seen the entity yet. Remember it and add a field for it + # into the environment object let obj = c.getEnvTypeForOwner(ow, n.info).skipTypes({tyRef, tyPtr}) - #getHiddenParam(owner).typ.skipTypes({tyRef, tyPtr}) addField(obj, s, c.graph.cache, c.idgen) + # create required upFields: var w = owner.skipGenericOwner if isInnerProc(w) or owner.isIterator: @@ -510,7 +483,6 @@ proc detectCapturedVars(n: PNode; owner: PSym; c: var DetectionPass) = """ let up = w.skipGenericOwner #echo "up for ", w.name.s, " up ", up.name.s - markAsClosure(c.graph, w, n) addClosureParam(c, w, n.info) # , ow createUpField(c, w, up, n.info) w = up @@ -532,7 +504,6 @@ type LiftingPass = object processed: IntSet envVars: Table[int, PNode] - inContainer: int proc initLiftingPass(fn: PSym): LiftingPass = result.processed = initIntSet() @@ -708,18 +679,15 @@ proc liftCapturedVars(n: PNode; owner: PSym; d: var DetectionPass; of nkSym: let s = n.sym if isInnerProc(s): - if not c.processed.containsOrIncl(s.id): + if s.typ.callConv == ccClosure and not c.processed.containsOrIncl(s.id): #if s.name.s == "temp": # echo renderTree(s.getBody, {renderIds}) - let oldInContainer = c.inContainer - c.inContainer = 0 var body = transformBody(d.graph, d.idgen, s, cache = false) body = liftCapturedVars(body, s, d, c) if c.envVars.getOrDefault(s.id).isNil: s.transformedBody = body else: s.transformedBody = newTree(nkStmtList, rawClosureCreation(s, d, c, n.info), body) - c.inContainer = oldInContainer if s.typ.callConv == ccClosure: result = symToClosure(n, owner, d, c) @@ -746,12 +714,9 @@ proc liftCapturedVars(n: PNode; owner: PSym; d: var DetectionPass; n[1] = x[1] of nkLambdaKinds, nkIteratorDef: if n.typ != nil and n[namePos].kind == nkSym: - let oldInContainer = c.inContainer - c.inContainer = 0 let m = newSymNode(n[namePos].sym) m.typ = n.typ result = liftCapturedVars(m, owner, d, c) - c.inContainer = oldInContainer of nkHiddenStdConv: if n.len == 2: n[1] = liftCapturedVars(n[1], owner, d, c) @@ -773,51 +738,11 @@ proc liftCapturedVars(n: PNode; owner: PSym; d: var DetectionPass; n[1] = liftCapturedVars(n[1], owner, d, c) return - let inContainer = n.kind in {nkObjConstr, nkBracket} - if inContainer: inc c.inContainer for i in 0.. don't run the + # pass + true + + proc liftLambdas*(g: ModuleGraph; fn: PSym, body: PNode; tooEarly: var bool; idgen: IdGenerator): PNode = # XXX backend == backendJs does not suffice! The compiletime stuff needs @@ -841,19 +783,38 @@ proc liftLambdas*(g: ModuleGraph; fn: PSym, body: PNode; tooEarly: var bool; # However we can do lifting for the stuff which is *only* compiletime. let isCompileTime = sfCompileTime in fn.flags or fn.kind == skMacro - if body.kind == nkEmpty or ( - g.config.backend == backendJs and not isCompileTime) or - fn.skipGenericOwner.kind != skModule: + if body.kind == nkEmpty or # forward declaration + (g.config.backend == backendJs and not isCompileTime) or + dontLift(fn): + # HACK: anonymous procedures defined outside of procedures (i.e. owned by + # a module), need a hidden environment parameter too, even though + # `nil` will always be passed. We add the parameter here, but + # non-nested anonymous closure procedures should just be disallowed + # instead + if fn.skipGenericOwner.kind == skModule and + fn.typ != nil and fn.typ.callConv == ccClosure: + # duplicates the code of ``addClosureParam``, but that's okay + let cp = newSym(skParam, getIdent(g.cache, paramName), nextSymId(idgen), fn, fn.info) + incl(cp.flags, sfFromGeneric) + + # setup the type: + cp.typ = newType(tyRef, nextTypeId(idgen), fn) + rawAddSon(cp.typ, createEnvObj(g, idgen, fn, fn.info)) + + addHiddenParam(fn, cp) - # ignore forward declaration: result = body tooEarly = true else: var d = initDetectionPass(g, fn, idgen) detectCapturedVars(body, fn, d) if not d.somethingToDo and fn.isIterator: + # the "lift captures" pass needs to always run either directly or + # indirectly for closure iterators, as it's also responsible for + # lifting locals inside a closure iterator into its environment addClosureParam(d, fn, body.info) d.somethingToDo = true + if d.somethingToDo: var c = initLiftingPass(fn) result = liftCapturedVars(body, fn, d, c) diff --git a/compiler/sem/sem.nim b/compiler/sem/sem.nim index c6e539061c9..43ec49e435f 100644 --- a/compiler/sem/sem.nim +++ b/compiler/sem/sem.nim @@ -66,7 +66,6 @@ import sigmatch, transf, aliases, - lambdalifting, sempass2, patterns, parampatterns, diff --git a/compiler/sem/semexprs.nim b/compiler/sem/semexprs.nim index 7cd74ab17e0..efe52ed6389 100644 --- a/compiler/sem/semexprs.nim +++ b/compiler/sem/semexprs.nim @@ -3363,11 +3363,6 @@ proc semExpr(c: PContext, n: PNode, flags: TExprFlags = {}): PNode = result.typ = s.typ else: result = semSym(c, n, s, flags) - - if c.matchedConcept == nil: - # XXX: concepts has a "pre-pass", so we need to guard the symbol capture - # for lambda lifting probably because it would mess something up? - semCaptureSym(s, c.p.owner) of nkSym: # because of the changed symbol binding, this does not mean that we # don't have to check the symbol for semantics here again! diff --git a/compiler/sem/sempass2.nim b/compiler/sem/sempass2.nim index 685328f7ae1..fbb64f194b6 100644 --- a/compiler/sem/sempass2.nim +++ b/compiler/sem/sempass2.nim @@ -63,12 +63,14 @@ import liftdestructors include sinkparameter_inference ##[ - -Second semantic checking pass over the AST. Necessary because the old -way had some inherent problems. Performs: +This module contains the second semantic checking pass over the AST. Necessary +because the old way had some inherent problems. Performs: - effect+exception tracking - "usage before definition" checking +- checking of captured entities +- detection of whether a procedure captures something and, if necessary, + adjusting the calling convention - also now calls the "lift destructor logic" at strategic positions, this is about to be put into the spec: @@ -82,6 +84,15 @@ For every sink parameter of type `T T` is marked. For every call `f()` the return type of `f()` is marked. +Captured entities +================= + +If a local is used inside procedure X but the local does not belong to X, the +local needs to be captured. Turning the procedure into a top-level procedure +with a hidden environment parameter is later performed by the `lambdalifting` +transformation, but semantic analysis already needs to know which procedures +require the `closure` calling convention, as this information is part of the +procedure's type. ]## @@ -379,6 +390,9 @@ proc listSideEffects(result: var SemReport; s: PSym; conf: ConfigRef; context: P result.sym = s listSideEffects(result, s, cycleCheck, conf, context, 1) +proc illegalCapture(s: PSym): bool {.inline.} = + result = classifyViewType(s.typ) != noView or s.kind == skResult + proc useVarNoInitCheck(a: PEffects; n: PNode; s: PSym) = if {sfGlobal, sfThread} * s.flags != {} and s.kind in {skVar, skLet} and s.magic != mNimvm: @@ -407,6 +421,18 @@ proc useVar(a: PEffects, n: PNode) = localReport(a.config, n.info, reportSym(rsemUninit, s)) # prevent superfluous warnings about the same variable: a.init.add s.id + + if s.kind in skLocalVars and + sfGlobal notin s.flags and # must not be a global + a.owner.kind in routineKinds and # only perform the check inside procedures + s.owner != a.owner and + illegalCapture(s): + # XXX: it'd be better if this error would be detected during the first + # semantic pass already, but detecting an access to an outer entity + # is not easily doable because of typed template/macro parameters + localReport(a.config, n.info, reportSym( + rsemIllegalMemoryCapture, s)) + useVarNoInitCheck(a, n, s) @@ -1486,6 +1512,119 @@ proc hasRealBody(s: PSym): bool = ## which is not a real implementation, refs #14314 result = {sfForward, sfImportc} * s.flags == {} +# ------------- routines for capture analysis ------------ + +proc detectCapture(owner, top: PSym, n: PNode, marker: var IntSet): PNode = + ## Traverses the imperative code in `n` and searches for entities (such as + ## locals) that need to be directly or indirectly captured. This is detected + ## by checking whether an entity is defined (i.e. owned) by a procedure that + ## is itself the owner of `top`. `owner` is the procedure the currently + ## processed AST belongs to. The node of the first encountered entity that + ## needs to be captured is returned. + ## + ## In the following case, ``inner`` indirectly captures something: + ## + ## .. code-block:: nim + ## + ## proc outer() = + ## var x = 0 + ## proc inner() = + ## proc innerInner() = + ## x = 1 + ## innerInner() + ## inner() + ## + ## On encountering the *usage* of a procedure that uses the ``.closure`` + ## calling convention (e.g. indirect call or assignment), ``detectCapture`` + ## recurses into the body of said procedure and checks if any captured entity + ## is defined outside of `top`. If that's the case, it means that something + ## that `top` captures was found. + template detect(s: PSym): PNode = + ## Checks if `s` is something that would need to be directly or indirectly + ## captured by `top`, and evaluates to `n` if yes + if top.isOwnedBy(s.owner): n + else: nil + + case n.kind + of nkTypeSection, nkTypeOfExpr, nkCommentStmt, nkIncludeStmt, nkImportStmt, + nkImportExceptStmt, nkExportStmt, nkExportExceptStmt, nkFromStmt, + nkStaticStmt, nkMixinStmt, nkBindStmt: + # XXX: this set of node kinds is commonly used. It would make sense to make + # a `const` out of it + discard "ignore declarative contexts" + of routineDefs: + discard "also ignore routine defs; we're only looking for alive code" + of nkSym: + let s = n.sym + case s.kind + of skLocalVars: + # make sure to not check globals, as those don't need to be captured + if sfGlobal notin s.flags and s.owner.id != owner.id: + # the local doesn't belong to the procedure we're analysing + result = detect(s) + of skProc, skFunc, skIterator: + # NOTE: a routine using the closure calling convention means that it + # *may* captures something (it might not). A routine not using the + # closure calling convention means that it *can't* capture anything, + # so we don't need to analyse the latter + if s.typ != nil and s.typ.callConv == ccClosure and + s.owner.kind != skModule: # don't analyse top-level closure iterators + if s.owner.id == owner.id: + # a procedure that's defined directly inside the currently analysed + # procedure is used as a value. Recurse into it to see if it captures + # an entity outside of `top` + result = detectCapture(s, top, s.ast[bodyPos], marker) + else: + # procedure A that uses the closure calling convention is used in + # procedure B, but A is not an inner procedure of B. Because of a + # limitation of the lambda-lifting implementation, B needs to be + # treated as capturing something + # XXX: fixing this requires two things: 1) tracking which routine + # really captures something, and 2) fixing ``lambdalifting`` + result = detect(s) + else: + discard "not relevant" + of nkWithoutSons - {nkSym}: + discard "not relevant" + of nkConv, nkHiddenStdConv, nkHiddenSubConv: + # only analyse the imperative part: + result = detectCapture(owner, top, n[1], marker) + of nkLambdaKinds: + result = detectCapture(owner, top, n[namePos], marker) + else: + for it in n.items: + result = detectCapture(owner, top, it, marker) + if result != nil: + # we've found something that requires capturing, don't continue and + # unwind + return + +proc isCompileTimeProc2*(s: PSym): bool = + ## Tests if `s` is a compile-time procedure, but compared to + ## ``isCompileTimeProc``, also considers where `s` is defined. In the + ## following example: + ## + ## .. code-block:: nim + ## + ## proc a() {.compileTime.} = + ## proc b() = + ## discard + ## + ## ``b`` is also a procedure that can only be run at compile-time, but it + ## doesn't have the ``sfCompileTime`` flag set + # TODO: propagate the ``sfCompileTime`` flag downwards instead (likely in + # sempass2) and use ``isCompileTimeProc`` directly + # FIXME: procedures defined inside ``static`` blocks are not detected + var s = s + while s.kind != skModule: + if isCompileTimeProc(s): + return true + s = s.skipGenericOwner + + result = false + +# ------------- public interface ---------------- + proc trackProc*(c: PContext; s: PSym, body: PNode) = addInNimDebugUtils(c.config, "trackProc") let g = c.graph @@ -1516,6 +1655,35 @@ proc trackProc*(c: PContext; s: PSym, body: PNode) = if typ.kind == tyOut and param.id notin t.init: message(g.config, param.info, warnProveInit, param.name.s) + let cap = block: + var marker: IntSet + detectCapture(s, s, body, marker) + + if cap != nil: + # the procedure captures something and thus requires a hidden environment + # parameter + if s.kind == skMacro or + (isCompileTimeProc2(s) and not isCompileTimeProc2(cap.sym.owner)): + # attempting to capture an entity that only exists at run-time in a + # compile-time context + localReport(g.config, cap.info, reportSym( + rsemIllegalCompTimeCapture, cap.sym)) + elif s.typ.callConv != ccClosure and tfExplicitCallConv in s.typ.flags: + # the analysed procedure is explicitly *not* using the ``.closure`` + # calling convention + localReport(g.config, cap.info, reportSymbols( + rsemIllegalCallconvCapture, @[cap.sym, s])) + else: + # set the calling convention and mark it as really capturing something: + s.typ.callConv = ccClosure + + elif s.kind != skIterator and s.typ.callConv == ccClosure: + # nothing is captured (so no hidden environment parameter is needed), but + # the procedure was explicitly annotated to use the ``.closure`` calling + # convention + assert tfExplicitCallConv in s.typ.flags + localReport(g.config, s.info, reportSym(rsemClosureWithoutEnv, s)) + if not isEmptyType(s.typ[0]) and (s.typ[0].requiresInit or s.typ[0].skipTypes(abstractInst).kind == tyVar) and s.kind in {skProc, skFunc, skConverter, skMethod}: diff --git a/compiler/sem/semstmts.nim b/compiler/sem/semstmts.nim index 4e36e286c83..23d947e6fb9 100644 --- a/compiler/sem/semstmts.nim +++ b/compiler/sem/semstmts.nim @@ -2874,9 +2874,7 @@ proc semIterator(c: PContext, n: PNode): PNode = # iterators are either 'inline' or 'closure'; for backwards compatibility, # we require first class iterators to be marked with 'closure' explicitly # -- at least for 0.9.2. - if s.typ.callConv == ccClosure: - incl(s.typ.flags, tfCapturesEnv) - else: + if s.typ.callConv != ccClosure: s.typ.callConv = ccInline if n[bodyPos].kind == nkEmpty and s.magic == mNone and c.inConceptDecl == 0: localReport(c.config, n.info, reportSym( diff --git a/compiler/vm/vmaux.nim b/compiler/vm/vmaux.nim index eae2701880b..341fa3a917b 100644 --- a/compiler/vm/vmaux.nim +++ b/compiler/vm/vmaux.nim @@ -96,7 +96,7 @@ func findMatchingBranch*(recCase: PNode, lit: PNode): int = func getEnvParam*(prc: PSym): PSym = ## Return the symbol the hidden environment parameter, or `nil` if there's ## none - if tfCapturesEnv in prc.typ.flags: + if prc.typ.callConv == ccClosure: lastSon(prc.ast[paramsPos]).sym else: nil diff --git a/compiler/vm/vmgen.nim b/compiler/vm/vmgen.nim index 43386f9aa28..1e2dea1a2d5 100644 --- a/compiler/vm/vmgen.nim +++ b/compiler/vm/vmgen.nim @@ -1783,11 +1783,6 @@ func setSlot(c: var TCtx; v: PSym): TRegister {.discardable.} = func cannotEval(c: TCtx; n: PNode) {.noinline, noreturn.} = raiseVmGenError(vmGenDiagCannotEvaluateAtComptime, n) -func isOwnedBy(a, b: PSym): bool = - var a = a.owner - while a != nil and a.kind != skModule: - if a == b: return true - a = a.owner func getOwner(c: TCtx): PSym = result = c.prc.sym @@ -2817,9 +2812,8 @@ proc genProcBody(c: var TCtx; s: PSym, body: PNode): int = if s.kind == skMacro and s.isGenericRoutineStrict: genGenericParams(c, s.ast[genericParamsPos]) - if tfCapturesEnv in s.typ.flags: - #let env = s.ast[paramsPos].lastSon.sym - #assert env.position == 2 + if s.typ.callConv == ccClosure: + # reserve a slot for the hidden environment parameter c.prc.regInfo.add RegInfo(refCount: 1, kind: slotFixedLet) gen(c, body) diff --git a/tests/lang_callable/closure/tclosure.nim b/tests/lang_callable/closure/tclosure.nim index 546d7026dd1..d516d03bec0 100644 --- a/tests/lang_callable/closure/tclosure.nim +++ b/tests/lang_callable/closure/tclosure.nim @@ -491,3 +491,9 @@ block tnoclosure: row = zip(row & @[0], @[0] & row).mapIt(it[0] + it[1]) echo row pascal(10) + +block non_nested_closure: + # make sure that a top-level anonymous closure procedure that works (for now) + var cl = proc (): int {.closure.} = 1 + doAssert cl is "closure" + doAssert cl() == 1 \ No newline at end of file diff --git a/tests/lang_callable/closure/tclosure_issues.nim b/tests/lang_callable/closure/tclosure_issues.nim index 4688834de94..0523b4f966c 100644 --- a/tests/lang_callable/closure/tclosure_issues.nim +++ b/tests/lang_callable/closure/tclosure_issues.nim @@ -80,3 +80,38 @@ block tissue7104: sp do: inc i echo "ok ", i + +block: + # a regression test against closure inference happening too early (i.e. + # during early overload resolution) + proc val(x: int): int = result + + proc test() = + var val = 1 # it's important that the name matches that of the procedure + proc inner(i: int) = + # because `val` is ambiguous, the ``val(i)`` expression is analysed by + # ``semIndirectOp``, which resolved `val` to the starting symbol to use + # for overload resolution via ``semExpr``. The closest symbol named + # `val` (that of the local in this case) is chosen, and due to capture + # analysis previously being run when an identifier was turned into a + # symbol, this meant that `inner` was erroneously inferred as capturing + # something + discard val(i) + + doAssert typeof(inner) isnot "closure" + inner(1) + + test() + +block: + # a regression test against closure inference modifying symbol state when + # being run inside a ``compiles`` context + proc test() = + var val = 0 + proc inner() = + doAssert compiles(val) + + doAssert typeof(inner) isnot "closure" + inner() + + test() \ No newline at end of file diff --git a/tests/lang_callable/closure/tillegal_comptime_capture.nim b/tests/lang_callable/closure/tillegal_comptime_capture.nim new file mode 100644 index 00000000000..6d50731024e --- /dev/null +++ b/tests/lang_callable/closure/tillegal_comptime_capture.nim @@ -0,0 +1,30 @@ +discard """ + description: ''' + Tests for detection of illegal captures across the compile-/run-time border + ''' + cmd: "nim check --msgFormat:sexp --filenames=canonical $options $file" + nimoutformat: sexp + action: reject +""" + +block: + # attempting to capture a run-time location inside a compile-time context is + # an error + proc outer() = + var x = 0 + proc inner() {.compileTime.} = + discard x #[tt.Error + ^ (SemIllegalCompTimeCapture)]# + + proc inner3() {.compileTime.} = + proc innerInner() = + discard x #[tt.Error + ^ (SemIllegalCompTimeCapture)]# + +block: + proc outer() {.compileTime.} = + proc innerInner() = # a compile-time procedure that's not explicitly + # marked as such + var y = 0 + proc innerInnerInner() {.compileTime.} = + discard y # legal; `innerInner` is also a compile-time procedure \ No newline at end of file diff --git a/tests/lang_callable/closure/tlate_inference.nim b/tests/lang_callable/closure/tlate_inference.nim new file mode 100644 index 00000000000..7331d19e24d --- /dev/null +++ b/tests/lang_callable/closure/tlate_inference.nim @@ -0,0 +1,87 @@ +discard """ + description: ''' + Test to make sure that both closure inference and lambda-lifting happen + late enough for typed template/macro parameters and 'gensym's to be + analysed too + ''' + targets: "c vm js" +""" + +import std/macros + +template validate(prc, val: untyped) = + ## Used by all test cases to check whether the expectations hold + doAssert typeof(a) is "closure" # did inference work? + a() # did lifting work? + doAssert val == 2 + +block template_params: + # test that closure inference and lambda-lifting work when the symbol comes + # from a typed template argument + proc test() = + template templ(val: typed) = + proc a() = + doAssert val == 1 + val = 2 + + validate(a, val) + + var x = 1 + templ(x) + + test() + +block macro_params: + # test that closure inference and lambda-lifting work when the symbol comes + # from a typed macro argument + proc test() = + macro m(val: typed) = + result = quote do: + proc a() = + doAssert `val` == 1 + `val` = 2 + + validate(a, `val`) + + var x = 1 + m(x) + + test() + +block template_gensym: + # test that closure inference and lambda-lifting work when the symbol is + # resolved during the first phase of template body symbol lookup (i.e. is + # a 'gensym') + proc test() = + template templ() = + var val = 1 # a gensym'ed symbol + + template nested() = + proc a() = + doAssert val == 1 + val = 2 + + validate(a, val) + + nested() + templ() + + test() + +block macro_gensym: + # test that closure inference and lamba-lifting work when the symbol is + # generated by a macro + proc test() = + macro m() = + let s = genSym(nskVar, "a") + result = quote do: + var `s` = 1 + proc a() = + doAssert `s` == 1 + `s` = 2 + + validate(a, `s`) + + m() + + test() \ No newline at end of file