From ef0c454791e75f9a1c1b86cc30f5e7c01fcb0edd Mon Sep 17 00:00:00 2001 From: Clyybber Date: Wed, 13 Mar 2024 12:21:16 +0100 Subject: [PATCH 01/17] Move cmpFloatRep from vm/vmgen to ast/trees --- compiler/ast/trees.nim | 10 ++++++++++ compiler/vm/vmgen.nim | 10 ---------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/compiler/ast/trees.nim b/compiler/ast/trees.nim index 317ec8896e7..4cc65f0e024 100644 --- a/compiler/ast/trees.nim +++ b/compiler/ast/trees.nim @@ -44,6 +44,16 @@ proc sameFloatIgnoreNan(a, b: BiggestFloat): bool {.inline.} = ## ignores NaN semantics, but ensures 0.0 == -0.0, see #13730 cast[uint64](a) == cast[uint64](b) or a == b +template cmpFloatRep*(a, b: BiggestFloat): bool = + ## Compares the bit-representation of floats `a` and `b` + # Special handling for floats, so that floats that have the same + # value but different bit representations are treated as different constants + cast[uint64](a) == cast[uint64](b) + # refs bug #16469 + # if we wanted to only distinguish 0.0 vs -0.0: + # if a.floatVal == 0.0: result = cast[uint64](a.floatVal) == cast[uint64](b.floatVal) + # else: result = a.floatVal == b.floatVal + template makeTreeEquivalenceProc*( name, relaxedKindCheck, symCheck, floatCheck, typeCheck, commentCheck) {.dirty.} = ## Defines a tree equivalence checking procedure. diff --git a/compiler/vm/vmgen.nim b/compiler/vm/vmgen.nim index 31ccf2a6f19..61bf4d32d48 100644 --- a/compiler/vm/vmgen.nim +++ b/compiler/vm/vmgen.nim @@ -798,16 +798,6 @@ proc rawGenLiteral(c: var TCtx, val: sink VmConstant): int = internalAssert c.config, result < regBxMax, "Too many constants used" -template cmpFloatRep(a, b: BiggestFloat): bool = - ## Compares the bit-representation of floats `a` and `b` - # Special handling for floats, so that floats that have the same - # value but different bit representations are treated as different constants - cast[uint64](a) == cast[uint64](b) - # refs bug #16469 - # if we wanted to only distinguish 0.0 vs -0.0: - # if a.floatVal == 0.0: result = cast[uint64](a.floatVal) == cast[uint64](b.floatVal) - # else: result = a.floatVal == b.floatVal - # Compares two trees for structural equality, also taking the type of # ``nkType`` nodes into account. This procedure is used to prevent the same # AST from being added as a node constant more than once From ce056014fc7ca15053d021ea85e2c1c4aa446a86 Mon Sep 17 00:00:00 2001 From: Clyybber Date: Wed, 13 Mar 2024 12:45:27 +0100 Subject: [PATCH 02/17] Use cmpFloatRep instead of float equality in sem/patterns --- compiler/sem/patterns.nim | 4 ++-- .../trmacros/trmacros_various2.nim | 13 +++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/compiler/sem/patterns.nim b/compiler/sem/patterns.nim index dead41b9ab9..84bfe6cfc61 100644 --- a/compiler/sem/patterns.nim +++ b/compiler/sem/patterns.nim @@ -63,7 +63,7 @@ proc sameKinds(a, b: PNode): bool {.inline.} = makeTreeEquivalenceProc(sameTrees, relaxedKindCheck = sameKinds(a, b), symCheck = a.sym == b.sym, - floatCheck = a.floatVal == b.floatVal, + floatCheck = cmpFloatRep(a.floatVal, b.floatVal), typeCheck = sameTypeOrNil(a.typ, b.typ), commentCheck = true # Ignore comments ) @@ -177,7 +177,7 @@ proc matches(c: PPatternContext, p, n: PNode): bool = of nkSym: result = p.sym == n.sym of nkIdent: result = p.ident.id == n.ident.id of nkIntLiterals: result = p.intVal == n.intVal - of nkFloatLiterals: result = p.floatVal == n.floatVal + of nkFloatLiterals: result = cmpFloatRep(p.floatVal, n.floatVal) of nkStrLiterals: result = p.strVal == n.strVal of nkEmpty, nkNilLit, nkType, nkCommentStmt: result = true # Ignore comments diff --git a/tests/lang_experimental/trmacros/trmacros_various2.nim b/tests/lang_experimental/trmacros/trmacros_various2.nim index 44941a02dff..e31afe25188 100644 --- a/tests/lang_experimental/trmacros/trmacros_various2.nim +++ b/tests/lang_experimental/trmacros/trmacros_various2.nim @@ -8,6 +8,8 @@ lo my awesome concat 1 TRM +10000000000.0 +-10000000000.0 ''' """ @@ -99,3 +101,14 @@ echo u * 3'u # 1 template dontAppendE{`&`(s, 'E')}(s: string): string = s var s = "T" echo s & 'E' & 'R' & 'M' + +# Floats must not be matched with float equality semantics +template capDivPos0{`/`(f, 0.0)}(f: float): float = + 10000000000.float + +template capDivNeg0{`/`(f, -0.0)}(f: float): float = + -10000000000.float + +let f = 1.0 +echo f / 0.0 # 10000000000.0 +echo f / -0.0 # -10000000000.0 From 835d0fa4116eb186589efd7378f8906dc62d02d1 Mon Sep 17 00:00:00 2001 From: Clyybber Date: Wed, 13 Mar 2024 13:04:55 +0100 Subject: [PATCH 03/17] Add comment about float equality in sem/guards --- compiler/sem/guards.nim | 3 +++ 1 file changed, 3 insertions(+) diff --git a/compiler/sem/guards.nim b/compiler/sem/guards.nim index bca5b672e4d..b4f469daf24 100644 --- a/compiler/sem/guards.nim +++ b/compiler/sem/guards.nim @@ -458,6 +458,9 @@ makeTreeEquivalenceProc(sameTree, symCheck = sameOpr(a.sym, b.sym) or (a.sym.magic != mNone and a.sym.magic == b.sym.magic), floatCheck = a.floatVal == b.floatVal, + # XXX: Even though float equality doesn't satisfy the + # substition property, sameTree is used as if it did. + # Using cmpFloatRep here would not be correct either. typeCheck = a.typ == b.typ, commentCheck = true # ignore comments ) From 87617b424e7fb63583d45c30f443f6fe7f3383f4 Mon Sep 17 00:00:00 2001 From: Clyybber Date: Wed, 13 Mar 2024 13:23:25 +0100 Subject: [PATCH 04/17] Use cmpFloatRep in trees.exprStructuralEquivalentStrictSymAndComm instead of sameFloatIgnoreNan --- compiler/ast/trees.nim | 2 +- tests/lang_callable/macros/tmacros_various.nim | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/compiler/ast/trees.nim b/compiler/ast/trees.nim index 4cc65f0e024..60c753e5564 100644 --- a/compiler/ast/trees.nim +++ b/compiler/ast/trees.nim @@ -106,7 +106,7 @@ export exprStructuralEquivalentStrictSym makeTreeEquivalenceProc(exprStructuralEquivalentStrictSymAndComm, relaxedKindCheck = false, symCheck = a.sym == b.sym, - floatCheck = sameFloatIgnoreNan(a.floatVal, b.floatVal), + floatCheck = cmpFloatRep(a.floatVal, b.floatVal), typeCheck = true, commentCheck = a.comment == b.comment ) diff --git a/tests/lang_callable/macros/tmacros_various.nim b/tests/lang_callable/macros/tmacros_various.nim index 5d7d62dffb1..96d91f60ab8 100644 --- a/tests/lang_callable/macros/tmacros_various.nim +++ b/tests/lang_callable/macros/tmacros_various.nim @@ -12,6 +12,8 @@ Infix macrocache ok CommentStmt "comment 1" CommentStmt "comment 2" +false +true ''' output: ''' @@ -346,3 +348,10 @@ block: static: echo treeRepr(C1[1]) echo treeRepr(C2[1]) + +block: + # Ensure float equality semantics are not used when comparing AST for equality + + static: + echo newLit(0.0) == newLit(-0.0) # false + echo newLit(NaN) == newLit(NaN) # true From 6217eb15c669a7123b03f9971d44e8dc34539c39 Mon Sep 17 00:00:00 2001 From: Clyybber Date: Wed, 13 Mar 2024 13:52:48 +0100 Subject: [PATCH 05/17] Use cmpFloatRep in trees.exprStructuralEquivalentStrictSymAndComm instead of sameFloatIgnoreNan. This does not cause a change in behaviour because only nkNilLit or nkSym nodes reach its only usage in sem/semfold for mEqProc --- compiler/ast/trees.nim | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/ast/trees.nim b/compiler/ast/trees.nim index 60c753e5564..92b5cee108a 100644 --- a/compiler/ast/trees.nim +++ b/compiler/ast/trees.nim @@ -97,7 +97,7 @@ export exprStructuralEquivalent makeTreeEquivalenceProc(exprStructuralEquivalentStrictSym, relaxedKindCheck = false, symCheck = a.sym == b.sym, - floatCheck = sameFloatIgnoreNan(a.floatVal, b.floatVal), + floatCheck = cmpFloatRep(a.floatVal, b.floatVal), typeCheck = true, commentCheck = true ) From e64f674a960deba23dcf9ff77dde9e189533b108 Mon Sep 17 00:00:00 2001 From: Clyybber Date: Wed, 13 Mar 2024 14:16:09 +0100 Subject: [PATCH 06/17] Don't use exprStructuralEquivalentStrictSym in sem/semfold and remove it from ast/trees as it's now deadcode --- compiler/ast/trees.nim | 9 --------- compiler/sem/semfold.nim | 13 +++++++++++-- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/compiler/ast/trees.nim b/compiler/ast/trees.nim index 92b5cee108a..9f7bda24781 100644 --- a/compiler/ast/trees.nim +++ b/compiler/ast/trees.nim @@ -94,15 +94,6 @@ makeTreeEquivalenceProc(exprStructuralEquivalent, ) export exprStructuralEquivalent -makeTreeEquivalenceProc(exprStructuralEquivalentStrictSym, - relaxedKindCheck = false, - symCheck = a.sym == b.sym, - floatCheck = cmpFloatRep(a.floatVal, b.floatVal), - typeCheck = true, - commentCheck = true -) -export exprStructuralEquivalentStrictSym - makeTreeEquivalenceProc(exprStructuralEquivalentStrictSymAndComm, relaxedKindCheck = false, symCheck = a.sym == b.sym, diff --git a/compiler/sem/semfold.nim b/compiler/sem/semfold.nim index 7a1125f3da2..5c4dfe35857 100644 --- a/compiler/sem/semfold.nim +++ b/compiler/sem/semfold.nim @@ -31,6 +31,7 @@ import ], compiler/front/[ options, + msgs, ], compiler/utils/[ platform, @@ -378,8 +379,16 @@ proc evalOp*(m: TMagic, n, a, b, c: PNode; idgen: IdGenerator; g: ModuleGraph): result = copyTree(a) result.typ = n.typ of mEqProc: - result = newIntNodeT(toInt128(ord( - exprStructuralEquivalentStrictSym(a, b))), n, idgen, g) + g.config.internalAssert(a.kind in {nkSym, nkNilLit} and + b.kind in {nkSym, nkNilLit}, + n.info, "mEqProc: invalid AST") + let isEqual = if a.kind != b.kind: + false + elif a.kind == nkSym: # and b.kind == nkSym: + a.sym == b.sym + else: # a.kind == nkNilLit and b.kind == nkNilLit + true + result = newIntNodeT(toInt128(ord(isEqual)), n, idgen, g) else: discard proc getConstIfExpr(c: PSym, n: PNode; idgen: IdGenerator; g: ModuleGraph): PNode = From 52a892887b8ec6441ec71ae907eaa98269efa1e8 Mon Sep 17 00:00:00 2001 From: Clyybber Date: Wed, 13 Mar 2024 15:17:10 +0100 Subject: [PATCH 07/17] Use cmpFloatRep in trees.exprStructuralEquivalent instead of sameFloatIgnoreNan and update and add tests to reflect this change in semantics. --- compiler/ast/trees.nim | 2 +- tests/errmsgs/tforwarddecl_defaultparam.nim | 9 +++++++++ tests/lang_callable/generics/tgenerics_issues.nim | 2 +- tests/statictypes/tstatictypes.nim | 15 +++++++++++++-- 4 files changed, 24 insertions(+), 4 deletions(-) create mode 100644 tests/errmsgs/tforwarddecl_defaultparam.nim diff --git a/compiler/ast/trees.nim b/compiler/ast/trees.nim index 9f7bda24781..e9ba4f4d42b 100644 --- a/compiler/ast/trees.nim +++ b/compiler/ast/trees.nim @@ -88,7 +88,7 @@ template makeTreeEquivalenceProc*( makeTreeEquivalenceProc(exprStructuralEquivalent, relaxedKindCheck = false, symCheck = a.sym.name.id == b.sym.name.id, # same symbol as string is enough - floatCheck = sameFloatIgnoreNan(a.floatVal, b.floatVal), + floatCheck = cmpFloatRep(a.floatVal, b.floatVal), typeCheck = true, commentCheck = true ) diff --git a/tests/errmsgs/tforwarddecl_defaultparam.nim b/tests/errmsgs/tforwarddecl_defaultparam.nim new file mode 100644 index 00000000000..17bf54a0e85 --- /dev/null +++ b/tests/errmsgs/tforwarddecl_defaultparam.nim @@ -0,0 +1,9 @@ +discard """ +errormsg: "overloaded 'reciprocal' leads to ambiguous calls" +line: 9 +""" + +# Differing float literal default args must prevent forward declaration +# and the compiler must not compare them via float equality +proc reciprocal(f: float = 0.0): float +proc reciprocal(f: float = -0.0): float = 1 / f diff --git a/tests/lang_callable/generics/tgenerics_issues.nim b/tests/lang_callable/generics/tgenerics_issues.nim index 010ade89b1f..d84039b0772 100644 --- a/tests/lang_callable/generics/tgenerics_issues.nim +++ b/tests/lang_callable/generics/tgenerics_issues.nim @@ -1084,4 +1084,4 @@ block typed_macro_in_generic_object_when: var o1 = Object[0]() doAssert not compiles(o1.val) var o2 = Object[1](val: 2) - doAssert o2.val == 2 \ No newline at end of file + doAssert o2.val == 2 diff --git a/tests/statictypes/tstatictypes.nim b/tests/statictypes/tstatictypes.nim index 54728cb969a..e8cf0c0db68 100644 --- a/tests/statictypes/tstatictypes.nim +++ b/tests/statictypes/tstatictypes.nim @@ -103,7 +103,11 @@ when true: block: # issue #13730 type Foo[T: static[float]] = object - doAssert Foo[0.0] is Foo[-0.0] + # It should not actually be considered the same type as + # float equality does not have the substition property, + # For example: 1 / 0.0 = Inf != -Inf = 1 / -0.0 + # even though 0.0 == -0.0 according to float semantics + doAssert Foo[0.0] isnot Foo[-0.0] when true: type @@ -411,4 +415,11 @@ block coercion_to_static_type: result = 2.1 # the call must be fully evaluated at compile-time - doAssert static[int](get()) == 1 \ No newline at end of file + doAssert static[int](get()) == 1 + + +proc reciprocal(f: static float): float = + 1 / f + +doAssert reciprocal(-0.0) == -Inf +doAssert reciprocal(0.0) == Inf From aad027545676ec088611a793c06fb223d6a04aa4 Mon Sep 17 00:00:00 2001 From: Clyybber Date: Wed, 13 Mar 2024 15:20:53 +0100 Subject: [PATCH 08/17] Remove trees.sameFloatIgnoreNan as it's deadcode and update comments --- compiler/ast/trees.nim | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/compiler/ast/trees.nim b/compiler/ast/trees.nim index e9ba4f4d42b..cf976150985 100644 --- a/compiler/ast/trees.nim +++ b/compiler/ast/trees.nim @@ -40,19 +40,13 @@ proc cyclicTree*(n: PNode): bool = var visited: seq[PNode] = @[] cyclicTreeAux(n, visited) -proc sameFloatIgnoreNan(a, b: BiggestFloat): bool {.inline.} = - ## ignores NaN semantics, but ensures 0.0 == -0.0, see #13730 - cast[uint64](a) == cast[uint64](b) or a == b - template cmpFloatRep*(a, b: BiggestFloat): bool = ## Compares the bit-representation of floats `a` and `b` # Special handling for floats, so that floats that have the same # value but different bit representations are treated as different constants + # As opposed to float equality this does not lack the substition or + # reflexivity property, which the compiler relies on for correctness. cast[uint64](a) == cast[uint64](b) - # refs bug #16469 - # if we wanted to only distinguish 0.0 vs -0.0: - # if a.floatVal == 0.0: result = cast[uint64](a.floatVal) == cast[uint64](b.floatVal) - # else: result = a.floatVal == b.floatVal template makeTreeEquivalenceProc*( name, relaxedKindCheck, symCheck, floatCheck, typeCheck, commentCheck) {.dirty.} = @@ -72,9 +66,6 @@ template makeTreeEquivalenceProc*( of nkIdent: result = a.ident.id == b.ident.id of nkIntLiterals: result = a.intVal == b.intVal of nkFloatLiterals: result = floatCheck - # XXX: Using float equality, even if partially tamed through - # sameFloatIgnoreNan, causes inconsistencies due to it - # lacking the substition and reflexivity property. of nkStrLiterals: result = a.strVal == b.strVal of nkType: result = typeCheck of nkCommentStmt: result = commentCheck From c8b2d8296110df74d358c2832924916112e9fbde Mon Sep 17 00:00:00 2001 From: Clyybber Date: Wed, 13 Mar 2024 15:25:58 +0100 Subject: [PATCH 09/17] Add comment about guards.sameTree invalid float equality --- compiler/ast/trees.nim | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/compiler/ast/trees.nim b/compiler/ast/trees.nim index cf976150985..6f4f9ef1ec4 100644 --- a/compiler/ast/trees.nim +++ b/compiler/ast/trees.nim @@ -49,7 +49,7 @@ template cmpFloatRep*(a, b: BiggestFloat): bool = cast[uint64](a) == cast[uint64](b) template makeTreeEquivalenceProc*( - name, relaxedKindCheck, symCheck, floatCheck, typeCheck, commentCheck) {.dirty.} = + name, relaxedKindCheck,symCheck, floatCheck, typeCheck, commentCheck) {.dirty.} = ## Defines a tree equivalence checking procedure. ## This skeleton is shared between all recursive ## `PNode` equivalence checks in the compiler code base @@ -66,6 +66,9 @@ template makeTreeEquivalenceProc*( of nkIdent: result = a.ident.id == b.ident.id of nkIntLiterals: result = a.intVal == b.intVal of nkFloatLiterals: result = floatCheck + # XXX: This should always use cmpFloatRep, see the comment in cmpFloatRep + # but sem/guards.sameTree still incorrectly uses this floatCheck param + # to use float equality instead. of nkStrLiterals: result = a.strVal == b.strVal of nkType: result = typeCheck of nkCommentStmt: result = commentCheck From 2cfd55a3c04049a77088f0dac37f274c06f4174c Mon Sep 17 00:00:00 2001 From: Clyybber Date: Wed, 13 Mar 2024 17:10:52 +0100 Subject: [PATCH 10/17] Undo whitespace change --- compiler/ast/trees.nim | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/ast/trees.nim b/compiler/ast/trees.nim index 6f4f9ef1ec4..6d9c629d728 100644 --- a/compiler/ast/trees.nim +++ b/compiler/ast/trees.nim @@ -49,7 +49,7 @@ template cmpFloatRep*(a, b: BiggestFloat): bool = cast[uint64](a) == cast[uint64](b) template makeTreeEquivalenceProc*( - name, relaxedKindCheck,symCheck, floatCheck, typeCheck, commentCheck) {.dirty.} = + name, relaxedKindCheck, symCheck, floatCheck, typeCheck, commentCheck) {.dirty.} = ## Defines a tree equivalence checking procedure. ## This skeleton is shared between all recursive ## `PNode` equivalence checks in the compiler code base From 06f63101ff7b6fa197b4bd878f05b64a0d01f157 Mon Sep 17 00:00:00 2001 From: Clyybber Date: Fri, 15 Mar 2024 13:13:19 +0100 Subject: [PATCH 11/17] exprStructuralEquivalentStrictSymAndComm: Don't ignore nkType type equality --- compiler/ast/trees.nim | 1 + .../lang_callable/macros/tmacros_various.nim | 20 +++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/compiler/ast/trees.nim b/compiler/ast/trees.nim index 317ec8896e7..187162bc138 100644 --- a/compiler/ast/trees.nim +++ b/compiler/ast/trees.nim @@ -98,6 +98,7 @@ makeTreeEquivalenceProc(exprStructuralEquivalentStrictSymAndComm, symCheck = a.sym == b.sym, floatCheck = sameFloatIgnoreNan(a.floatVal, b.floatVal), typeCheck = true, + typeCheck = a.typ == b.typ, commentCheck = a.comment == b.comment ) export exprStructuralEquivalentStrictSymAndComm diff --git a/tests/lang_callable/macros/tmacros_various.nim b/tests/lang_callable/macros/tmacros_various.nim index 5d7d62dffb1..f75b1cbaa66 100644 --- a/tests/lang_callable/macros/tmacros_various.nim +++ b/tests/lang_callable/macros/tmacros_various.nim @@ -12,6 +12,7 @@ Infix macrocache ok CommentStmt "comment 1" CommentStmt "comment 2" +false ''' output: ''' @@ -346,3 +347,22 @@ block: static: echo treeRepr(C1[1]) echo treeRepr(C2[1]) + +block: + # Ensure nkType equality is not ignored by `==` for NimNode + macro checkEq(a, b: typed) = + echo a == b + + type Exception1 = object of Exception + type Exception2 = object of Exception + checkEq (; + try: + discard + except Exception1: + discard + ), (; + try: + discard + except Exception2: + discard + ) From f9f8fd7648c186beac1152d9c57366a3b9465ee8 Mon Sep 17 00:00:00 2001 From: Clyybber Date: Fri, 15 Mar 2024 13:51:01 +0100 Subject: [PATCH 12/17] Fix diff issue --- compiler/ast/trees.nim | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/ast/trees.nim b/compiler/ast/trees.nim index 187162bc138..24473ece706 100644 --- a/compiler/ast/trees.nim +++ b/compiler/ast/trees.nim @@ -97,7 +97,6 @@ makeTreeEquivalenceProc(exprStructuralEquivalentStrictSymAndComm, relaxedKindCheck = false, symCheck = a.sym == b.sym, floatCheck = sameFloatIgnoreNan(a.floatVal, b.floatVal), - typeCheck = true, typeCheck = a.typ == b.typ, commentCheck = a.comment == b.comment ) From e8bf6c75827816c81cf2f258e1223ba82bf651ac Mon Sep 17 00:00:00 2001 From: Clyybber Date: Fri, 15 Mar 2024 15:13:36 +0100 Subject: [PATCH 13/17] Remove cmpNodeCnst as it's equal to exprStructuralEquivalentStrictSymAndComm --- compiler/vm/vmgen.nim | 13 +------------ tests/lang_callable/macros/tmacros_various.nim | 4 ++-- 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/compiler/vm/vmgen.nim b/compiler/vm/vmgen.nim index 61bf4d32d48..38d499be94d 100644 --- a/compiler/vm/vmgen.nim +++ b/compiler/vm/vmgen.nim @@ -798,17 +798,6 @@ proc rawGenLiteral(c: var TCtx, val: sink VmConstant): int = internalAssert c.config, result < regBxMax, "Too many constants used" -# Compares two trees for structural equality, also taking the type of -# ``nkType`` nodes into account. This procedure is used to prevent the same -# AST from being added as a node constant more than once -makeTreeEquivalenceProc(cmpNodeCnst, - relaxedKindCheck = false, - symCheck = a.sym == b.sym, - floatCheck = cmpFloatRep(a.floatVal, b.floatVal), - typeCheck = a.typ == b.typ, - commentCheck = a.comment == b.comment -) - template makeCnstFunc(name, vType, aKind, valName, cmp) {.dirty.} = proc name(c: var TCtx, val: vType): int = for (i, cnst) in c.constants.pairs(): @@ -818,7 +807,7 @@ template makeCnstFunc(name, vType, aKind, valName, cmp) {.dirty.} = c.rawGenLiteral: VmConstant(kind: aKind, valName: val) -makeCnstFunc(toNodeCnst, PNode, cnstNode, node, cmpNodeCnst) +makeCnstFunc(toNodeCnst, PNode, cnstNode, node, exprStructuralEquivalentStrictSymAndComm) makeCnstFunc(toIntCnst, BiggestInt, cnstInt, intVal, `==`) diff --git a/tests/lang_callable/macros/tmacros_various.nim b/tests/lang_callable/macros/tmacros_various.nim index 6cacd9335fb..674e1347342 100644 --- a/tests/lang_callable/macros/tmacros_various.nim +++ b/tests/lang_callable/macros/tmacros_various.nim @@ -335,8 +335,8 @@ block: # bug #15118 flop("b") block: - # Ensure nkCommentStmt equality is not ignored when vmgen.cmpNodeCnst - # is used to deduplicate NimNode constants, so that `CommentStmt "comment 2"` + # Ensure nkCommentStmt equality is not ignored when vmgen.toNodeCnst + # deduplicates NimNode constants, so that `CommentStmt "comment 2"` # is not counted as a duplicate of `CommentStmt "comment 1"` and # incorrectly optimized to point at the `Comment "comment 1"` node From 97615b26e974074564e1ea1d8f142acb89a0b9b4 Mon Sep 17 00:00:00 2001 From: Clyybber Date: Fri, 15 Mar 2024 15:16:48 +0100 Subject: [PATCH 14/17] Use bit equality in sem/guards --- compiler/sem/guards.nim | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/compiler/sem/guards.nim b/compiler/sem/guards.nim index b4f469daf24..3410ac5a2ba 100644 --- a/compiler/sem/guards.nim +++ b/compiler/sem/guards.nim @@ -454,13 +454,12 @@ proc sameOpr(a, b: PSym): bool = else: result = a == b makeTreeEquivalenceProc(sameTree, + # XXX: This completely ignores that expressions might + # not be pure/deterministic. relaxedKindCheck = false, symCheck = sameOpr(a.sym, b.sym) or (a.sym.magic != mNone and a.sym.magic == b.sym.magic), - floatCheck = a.floatVal == b.floatVal, - # XXX: Even though float equality doesn't satisfy the - # substition property, sameTree is used as if it did. - # Using cmpFloatRep here would not be correct either. + floatCheck = cmpFloatRep(a.floatVal, b.floatVal), typeCheck = a.typ == b.typ, commentCheck = true # ignore comments ) From 9377a1b3d3523fe5233a95cdfcd9723a6dae2f7d Mon Sep 17 00:00:00 2001 From: Clyybber Date: Fri, 15 Mar 2024 15:19:01 +0100 Subject: [PATCH 15/17] Always use cmpFloatRep in makeTreeEquivalenceProc --- compiler/ast/trees.nim | 9 ++------- compiler/sem/guards.nim | 1 - compiler/sem/patterns.nim | 1 - 3 files changed, 2 insertions(+), 9 deletions(-) diff --git a/compiler/ast/trees.nim b/compiler/ast/trees.nim index 46938cd4494..419f60b9e8b 100644 --- a/compiler/ast/trees.nim +++ b/compiler/ast/trees.nim @@ -49,7 +49,7 @@ template cmpFloatRep*(a, b: BiggestFloat): bool = cast[uint64](a) == cast[uint64](b) template makeTreeEquivalenceProc*( - name, relaxedKindCheck, symCheck, floatCheck, typeCheck, commentCheck) {.dirty.} = + name, relaxedKindCheck, symCheck, typeCheck, commentCheck) {.dirty.} = ## Defines a tree equivalence checking procedure. ## This skeleton is shared between all recursive ## `PNode` equivalence checks in the compiler code base @@ -65,10 +65,7 @@ template makeTreeEquivalenceProc*( of nkSym: result = symCheck of nkIdent: result = a.ident.id == b.ident.id of nkIntLiterals: result = a.intVal == b.intVal - of nkFloatLiterals: result = floatCheck - # XXX: This should always use cmpFloatRep, see the comment in cmpFloatRep - # but sem/guards.sameTree still incorrectly uses this floatCheck param - # to use float equality instead. + of nkFloatLiterals: result = cmpFloatRep(a.floatVal, b.floatVal) of nkStrLiterals: result = a.strVal == b.strVal of nkType: result = typeCheck of nkCommentStmt: result = commentCheck @@ -82,7 +79,6 @@ template makeTreeEquivalenceProc*( makeTreeEquivalenceProc(exprStructuralEquivalent, relaxedKindCheck = false, symCheck = a.sym.name.id == b.sym.name.id, # same symbol as string is enough - floatCheck = cmpFloatRep(a.floatVal, b.floatVal), typeCheck = true, commentCheck = true ) @@ -91,7 +87,6 @@ export exprStructuralEquivalent makeTreeEquivalenceProc(exprStructuralEquivalentStrictSymAndComm, relaxedKindCheck = false, symCheck = a.sym == b.sym, - floatCheck = cmpFloatRep(a.floatVal, b.floatVal), typeCheck = a.typ == b.typ, commentCheck = a.comment == b.comment ) diff --git a/compiler/sem/guards.nim b/compiler/sem/guards.nim index 3410ac5a2ba..aa4df7b0458 100644 --- a/compiler/sem/guards.nim +++ b/compiler/sem/guards.nim @@ -459,7 +459,6 @@ makeTreeEquivalenceProc(sameTree, relaxedKindCheck = false, symCheck = sameOpr(a.sym, b.sym) or (a.sym.magic != mNone and a.sym.magic == b.sym.magic), - floatCheck = cmpFloatRep(a.floatVal, b.floatVal), typeCheck = a.typ == b.typ, commentCheck = true # ignore comments ) diff --git a/compiler/sem/patterns.nim b/compiler/sem/patterns.nim index 84bfe6cfc61..40406e7bf8b 100644 --- a/compiler/sem/patterns.nim +++ b/compiler/sem/patterns.nim @@ -63,7 +63,6 @@ proc sameKinds(a, b: PNode): bool {.inline.} = makeTreeEquivalenceProc(sameTrees, relaxedKindCheck = sameKinds(a, b), symCheck = a.sym == b.sym, - floatCheck = cmpFloatRep(a.floatVal, b.floatVal), typeCheck = sameTypeOrNil(a.typ, b.typ), commentCheck = true # Ignore comments ) From 7a8d029ca544fc31c6328c7c4aee8b6b6636206b Mon Sep 17 00:00:00 2001 From: Clyybber Date: Fri, 15 Mar 2024 15:20:20 +0100 Subject: [PATCH 16/17] Don't export patterns.sameTrees --- compiler/sem/patterns.nim | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/sem/patterns.nim b/compiler/sem/patterns.nim index 40406e7bf8b..27e28c4fbb8 100644 --- a/compiler/sem/patterns.nim +++ b/compiler/sem/patterns.nim @@ -66,7 +66,6 @@ makeTreeEquivalenceProc(sameTrees, typeCheck = sameTypeOrNil(a.typ, b.typ), commentCheck = true # Ignore comments ) -export sameTrees proc inSymChoice(sc, x: PNode): bool = if sc.kind == nkClosedSymChoice: From a628d358767fec5fa819e0d7ceb2962e582544b8 Mon Sep 17 00:00:00 2001 From: Clyybber Date: Sat, 16 Mar 2024 13:10:25 +0100 Subject: [PATCH 17/17] Expand test with saems suggestion --- .../lang_callable/macros/tmacros_various.nim | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/lang_callable/macros/tmacros_various.nim b/tests/lang_callable/macros/tmacros_various.nim index f75b1cbaa66..39802860453 100644 --- a/tests/lang_callable/macros/tmacros_various.nim +++ b/tests/lang_callable/macros/tmacros_various.nim @@ -13,6 +13,7 @@ macrocache ok CommentStmt "comment 1" CommentStmt "comment 2" false +false ''' output: ''' @@ -366,3 +367,22 @@ block: except Exception2: discard ) + + macro checkEqOfTry(a, b: typed) = + echo a[0][1][1] == b[0][1][1] + + checkEqOfTry (; + block: + type E = object of Exception1 + try: + discard + except E: + discard + ), (; + block: + type E = object of Exception2 + try: + discard + except E: + discard + )