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.

Sign up for GitHub

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

fix #13502 a and b now short-circuits semcheck in VM #13541

Closed
wants to merge 1 commit into from
Closed

fix #13502 a and b now short-circuits semcheck in VM #13541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions compiler/semexprs.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2153,6 +2153,25 @@ proc semMagic(c: PContext, n: PNode, s: PSym, flags: TExprFlags): PNode =
result[0] = newSymNode(s, n[0].info)
result[1] = semAddrArg(c, n[1], s.name.s == "unsafeAddr")
result.typ = makePtrType(c, result[1].typ)
of mBitandI:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too hacky. I understand why you need to do it this way but I wonder if there is a better way...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to fix this line instead:

  # semfold.nim
  of mBitandI, mAnd: result = newIntNodeT(bitand(a.getInt, b.getInt), n, g)

Or to add logic to semmagic.magicsAfterOverloadResolution? Sem'checking both operands for and and or does make sense.

Copy link
Member Author

@timotheecour timotheecour Mar 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and or

good point, or should also short-circuit, I will update PR after the design for and is accepted (should be straightforward to adapt)

But for the remaining points:

How could that work? if you semcheck the 2nd operand for and , const a = false and nonexistant would not compile; nor would when declared(Foo) and T is Foo: discard

Error: undeclared identifier would be issued before code reaches semmagic.magicsAfterOverloadResolution or semfold.evalOp.semcheck, which is why I'm doing it instead in semMagic

If you nim c -r main with:

static:echo "ok1"
const a = false and nonexistant

and you add

# semfold.nim
  of mBitandI, mAnd: 
    echo ("semfold", g.config$a.info,)
    ...

and

# semmagic
proc magicsAfterOverloadResolution(c: PContext, n: PNode, flags: TExprFlags): PNode =
  echo ("magicsAfterOverloadResolution", c.config$n.info,)
  ...

you'll see:

ok1
main.nim: Error: undeclared identifier: 'nonexistant'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How could that work? if you semcheck the 2nd operand for and , const a = false and nonexistant would not compile; nor would when declared(Foo) and T is Foo: discard
Error: undeclared identifier would be issued before code reaches

Exactly. As it should be. I'm sorry, I seem to have misunderstood this point previously. Short cut evaluation refers to evaluation, not to the semantic checking of and and or.

Copy link
Member Author

@timotheecour timotheecour Mar 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you seemed to agree here #13502 (comment) it should be legal

This is a bug, not a feature request. In fact, it's a duplicate of some other bug report.

feature or bug is a matter of perspective, here's a motivating example from https://github.com/nim-lang/Nim/pull/13498/files#diff-7eb4d1654e7ba69a150250a34148c9c9

template ctAnd(a, b): bool =
  # pending https://github.com/nim-lang/Nim/issues/13502
  when a:
    when b: true
    else: false
  else: false

template initImpl(result: typed, size: int) =
  when ctAnd(declared(SharedTable), type(result) is SharedTable):
    init(result, size)
  else: ...

or, without ctAnd you'd need to duplicate the else branch (worst option) or write boilerplate:

template initImpl(result: typed, size: int) =
  template fallback() = ...
  when declared(SharedTable):
    when type(result) is SharedTable:
      init(result, size)
    else:
      fallback()
  else:
    fallback()

with this PR you'd simply write:

template initImpl(result: typed, size: int) =
  when declared(SharedTable) and type(result) is SharedTable:
    init(result, size)
  else: ...

which is arguably simpler. Are you implying #13502 should be closed as wontfix? I don't see downsides and only upsides to making when declared(Foo) and T is Foo work. If it can't be accepted, then we should add ctAnd, ctOr to stdlib

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which is arguably simpler.

Well it's simpler to use but a weird special case.

Copy link
Member Author

@timotheecour timotheecour Mar 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note that I ensured it only affects static/when/const blocks, not general VM code, so that:

template fun() = # ditto with generics, procs etc
  let a = b and c # uses full semcheck despite running inside VM
  const a2 = b and c # uses short-circuit (ditto with when b and c)
static: fun()

seems reasonable feature, if feature is accepted I can make sure doc is clear

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but I'm really happier without this special case, regardless of "only const/when/static blocks" are affected or not.

Copy link
Member Author

@timotheecour timotheecour Mar 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:-( would you accept a modified PR that adds insteadsugar.lazyAnd, sugar.lazyOr ?

template lazyAnd*(lhs: bool, rhs: untyped): bool =
  when lhs: rhs
  else: false

and analog for lazyOr

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said, do not circumvent the RFC process. lazyAnd is a good name, but consider that your very own implementation is a single when statement, why not write it as this when the need arises:

when (when declared(SharedTable): (typeof(result) is SharedTable) else: false):
  code here

# somehow, `true and true` falls here and not under mAnd, IIUC this is because
# magic overloads (`and`) don't do overload resolution since semcheck
# hasn't been done yet
if c.inStaticContext > 0:
let lhs = semExprWithType(c, n[1])
if lhs.typ.kind == tyBool:
result = newTree(nkCall)
result.add newIdentNode(getIdent(c.cache, "nimInternalAndVM"), n.info)
result.add lhs
result.add n[2]
result = semExpr(c, result)
else:
let n2 = copyNode(n)
n2.sons = n.sons
n2[1] = lhs
result = semDirectOp(c, n2, flags)
else:
result = semDirectOp(c, n, flags)
of mTypeOf:
markUsed(c, n.info, s)
result = semTypeOf(c, n)
Expand Down
7 changes: 7 additions & 0 deletions lib/system/basic_types.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,13 @@ proc `and`*(x, y: bool): bool {.magic: "And", noSideEffect.}
## are true).
##
## Evaluation is lazy: if ``x`` is false, ``y`` will not even be evaluated.
## Semantic check is lazy in VM, to allow `when declared(Foo) and T is Foo`

template nimInternalAndVM*(x: bool, y: untyped): bool =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new way of "fixing" things by adding implementation details to system.nim is not acceptable.

## Internal lowering used by `and` in VM to enable shortcuiting semcheck
when x: y
else: false

proc `or`*(x, y: bool): bool {.magic: "Or", noSideEffect.}
## Boolean ``or``; returns true if ``not (not x and not y)`` (if any of
## the arguments is true).
Expand Down
17 changes: 17 additions & 0 deletions tests/system/tsystem_misc.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,3 +210,20 @@ block: # Ordinal
# doAssert enum is Ordinal # fails
# doAssert Ordinal is SomeOrdinal
# doAssert SomeOrdinal is Ordinal

block: # and VM shortcircuits semcheck
doAssert not compiles(nonexistant)
const a1 = false and nonexistant
doAssert not a1
const a2 = true and false
doAssert not a2
const a3 = true and true
doAssert a3

static:
const a4 = false and nonexistant
doAssert not a4
let a5 = false and nonexistant # still works, we're inside a static context
doAssert not a5
const a6 = 2 and 3 # make sure we don't break bitwise and
doAssert a6 == 2