From de5c0d3aa9fd713a698f7f596c9bf925d00729c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oscar=20Nihlg=C3=A5rd?= Date: Fri, 10 May 2019 11:10:11 +0200 Subject: [PATCH] Make range checks in semConv (#7164) * Remove NaN/Inf/NegInf magic * Make range checks in semConv * fix the failing line * fix `firstOrd` and `lastOrd` * fix `localError` * remove debug comment * Cleanup, fix failing test * make tests green --- changelog.md | 1 + compiler/ast.nim | 1 - compiler/semexprs.nim | 53 ++++++++++++++++------- compiler/semfold.nim | 5 +-- compiler/types.nim | 16 +++++++ lib/posix/posix_nintendoswitch_consts.nim | 2 +- lib/pure/segfaults.nim | 2 +- lib/windows/winlean.nim | 6 +-- tests/misc/tconv.nim | 43 ++++++++++++++++++ 9 files changed, 103 insertions(+), 26 deletions(-) create mode 100644 tests/misc/tconv.nim diff --git a/changelog.md b/changelog.md index 91b8f5bbdd..385c8fecfa 100644 --- a/changelog.md +++ b/changelog.md @@ -34,6 +34,7 @@ - To use multi-methods, explicit `--multimethods:on` is now needed. +- Compile time checks for integer and float conversions are now stricter. For example, `const x = uint32(-1)` now gives a compile time error instead of being equivalent to `const x = 0xFFFFFFFF'u32`. #### Breaking changes in the standard library diff --git a/compiler/ast.nim b/compiler/ast.nim index ee90c566bb..76665809a4 100644 --- a/compiler/ast.nim +++ b/compiler/ast.nim @@ -656,7 +656,6 @@ type mVoidType, mPNimrodNode, mShared, mGuarded, mLock, mSpawn, mDeepCopy, mIsMainModule, mCompileDate, mCompileTime, mProcCall, mCpuEndian, mHostOS, mHostCPU, mBuildOS, mBuildCPU, mAppType, - mNaN, mInf, mNegInf, mCompileOption, mCompileOptionArg, mNLen, mNChild, mNSetChild, mNAdd, mNAddMultiple, mNDel, mNKind, mNSymKind, diff --git a/compiler/semexprs.nim b/compiler/semexprs.nim index acdece3fd8..9b36d41ec8 100644 --- a/compiler/semexprs.nim +++ b/compiler/semexprs.nim @@ -99,7 +99,8 @@ type TConvStatus = enum convOK, convNotNeedeed, - convNotLegal + convNotLegal, + convNotInRange proc checkConversionBetweenObjects(castDest, src: PType; pointers: int): TConvStatus = let diff = inheritanceDiff(castDest, src) @@ -111,18 +112,16 @@ proc checkConversionBetweenObjects(castDest, src: PType; pointers: int): TConvSt const IntegralTypes = {tyBool, tyEnum, tyChar, tyInt..tyUInt64} -proc checkConvertible(c: PContext, castDest, src: PType): TConvStatus = +proc checkConvertible(c: PContext, targetTyp: PType, src: PNode): TConvStatus = + let srcTyp = src.typ.skipTypes({tyStatic}) result = convOK - # We're interested in the inner type and not in the static tag - var src = src.skipTypes({tyStatic}) - if sameType(castDest, src) and castDest.sym == src.sym: + if sameType(targetTyp, srcTyp) and targetTyp.sym == srcTyp.sym: # don't annoy conversions that may be needed on another processor: - if castDest.kind notin IntegralTypes+{tyRange}: + if targetTyp.kind notin IntegralTypes+{tyRange}: result = convNotNeedeed return - # Save for later - var d = skipTypes(castDest, abstractVar) - var s = src + var d = skipTypes(targetTyp, abstractVar) + var s = srcTyp if s.kind in tyUserTypeClasses and s.isResolvedUserTypeClass: s = s.lastSon s = skipTypes(s, abstractVar-{tyTypeDesc, tyOwned}) @@ -138,19 +137,36 @@ proc checkConvertible(c: PContext, castDest, src: PType): TConvStatus = d = d.lastSon s = s.lastSon inc pointers + + let targetBaseTyp = skipTypes(targetTyp, abstractVarRange) + let srcBaseTyp = skipTypes(srcTyp, abstractVarRange-{tyTypeDesc}) + if d == nil: result = convNotLegal elif d.kind == tyObject and s.kind == tyObject: result = checkConversionBetweenObjects(d, s, pointers) - elif (skipTypes(castDest, abstractVarRange).kind in IntegralTypes) and - (skipTypes(src, abstractVarRange-{tyTypeDesc}).kind in IntegralTypes): - # accept conversion between integral types - discard + elif (targetBaseTyp.kind in IntegralTypes) and + (srcBaseTyp.kind in IntegralTypes): + if targetTyp.isOrdinalType: + if src.kind in nkCharLit..nkUInt64Lit and + src.intVal notin firstOrd(c.config, targetTyp)..lastOrd(c.config, targetTyp): + result = convNotInRange + elif src.kind in nkFloatLit..nkFloat64Lit and + (classify(src.floatVal) in {fcNaN, fcNegInf, fcInf} or + src.floatVal.int64 notin firstOrd(c.config, targetTyp)..lastOrd(c.config, targetTyp)): + result = convNotInRange + elif targetBaseTyp.kind in tyFloat..tyFloat64: + if src.kind in nkFloatLit..nkFloat64Lit and + not floatRangeCheck(src.floatVal, targetTyp): + result = convNotInRange + elif src.kind in nkCharLit..nkUInt64Lit and + not floatRangeCheck(src.intval.float, targetTyp): + result = convNotInRange else: # we use d, s here to speed up that operation a bit: case cmpTypes(c, d, s) of isNone, isGeneric: - if not compareTypes(castDest.skipTypes(abstractVar), src.skipTypes({tyOwned}), dcEqIgnoreDistinct): + if not compareTypes(targetTyp.skipTypes(abstractVar), srcTyp.skipTypes({tyOwned}), dcEqIgnoreDistinct): result = convNotLegal else: discard @@ -260,7 +276,7 @@ proc semConv(c: PContext, n: PNode): PNode = addSon(result, op) if not isSymChoice(op): - let status = checkConvertible(c, result.typ, op.typ) + let status = checkConvertible(c, result.typ, op) case status of convOK: # handle SomeProcType(SomeGenericProc) @@ -275,10 +291,15 @@ proc semConv(c: PContext, n: PNode): PNode = if result == nil: localError(c.config, n.info, "illegal conversion from '$1' to '$2'" % [op.typ.typeToString, result.typ.typeToString]) + of convNotInRange: + let value = + if op.kind in {nkCharLit..nkUInt64Lit}: $op.getInt else: $op.getFloat + localError(c.config, n.info, errGenerated, value & " can't be converted to " & + result.typ.typeToString) else: for i in 0 ..< sonsLen(op): let it = op.sons[i] - let status = checkConvertible(c, result.typ, it.typ) + let status = checkConvertible(c, result.typ, it) if status in {convOK, convNotNeedeed}: markUsed(c.config, n.info, it.sym, c.graph.usageSym) onUse(n.info, it.sym) diff --git a/compiler/semfold.nim b/compiler/semfold.nim index 4a419a726f..824d181269 100644 --- a/compiler/semfold.nim +++ b/compiler/semfold.nim @@ -559,9 +559,6 @@ proc getConstExpr(m: PSym, n: PNode; g: ModuleGraph): PNode = of mBuildOS: result = newStrNodeT(toLowerAscii(platform.OS[g.config.target.hostOS].name), n, g) of mBuildCPU: result = newStrNodeT(platform.CPU[g.config.target.hostCPU].name.toLowerAscii, n, g) of mAppType: result = getAppType(n, g) - of mNaN: result = newFloatNodeT(NaN, n, g) - of mInf: result = newFloatNodeT(Inf, n, g) - of mNegInf: result = newFloatNodeT(NegInf, n, g) of mIntDefine: if isDefined(g.config, s.name.s): try: @@ -733,7 +730,7 @@ proc getConstExpr(m: PSym, n: PNode; g: ModuleGraph): PNode = var a = getConstExpr(m, n.sons[1], g) if a == nil: return # XXX: we should enable `check` for other conversion types too - result = foldConv(n, a, g, check=n.kind == nkHiddenSubConv) + result = foldConv(n, a, g, check=n.kind == nkHiddenStdConv) of nkCast: var a = getConstExpr(m, n.sons[1], g) if a == nil: return diff --git a/compiler/types.nim b/compiler/types.nim index d56794d2dd..f702a75211 100644 --- a/compiler/types.nim +++ b/compiler/types.nim @@ -741,6 +741,22 @@ proc lastFloat*(t: PType): BiggestFloat = internalError(newPartialConfigRef(), "invalid kind for lastFloat(" & $t.kind & ')') NaN +proc floatRangeCheck*(x: BiggestFloat, t: PType): bool = + case t.kind + # This needs to be special cased since NaN is never + # part of firstFloat(t) .. lastFloat(t) + of tyFloat..tyFloat128: + true + of tyRange: + x in firstFloat(t) .. lastFloat(t) + of tyVar: + floatRangeCheck(x, t.sons[0]) + of tyGenericInst, tyDistinct, tyTypeDesc, tyAlias, tySink, + tyStatic, tyInferred, tyUserTypeClasses: + floatRangeCheck(x, lastSon(t)) + else: + internalError(newPartialConfigRef(), "invalid kind for floatRangeCheck:" & $t.kind) + false proc lengthOrd*(conf: ConfigRef; t: PType): BiggestInt = case t.skipTypes(tyUserTypeClasses).kind diff --git a/lib/posix/posix_nintendoswitch_consts.nim b/lib/posix/posix_nintendoswitch_consts.nim index 1e782d92e7..8cd8824b53 100644 --- a/lib/posix/posix_nintendoswitch_consts.nim +++ b/lib/posix/posix_nintendoswitch_consts.nim @@ -243,7 +243,7 @@ const IPPROTO_TCP* = cint(6) const IPPROTO_UDP* = cint(17) const INADDR_ANY* = InAddrScalar(0) const INADDR_LOOPBACK* = InAddrScalar(2130706433) -const INADDR_BROADCAST* = InAddrScalar(-1) +const INADDR_BROADCAST* = InAddrScalar(4294967295) const INET_ADDRSTRLEN* = cint(16) const INET6_ADDRSTRLEN* = cint(46) const IPV6_JOIN_GROUP* = cint(12) diff --git a/lib/pure/segfaults.nim b/lib/pure/segfaults.nim index 3735084302..354c431a3b 100644 --- a/lib/pure/segfaults.nim +++ b/lib/pure/segfaults.nim @@ -25,7 +25,7 @@ when defined(windows): import winlean const - EXCEPTION_ACCESS_VIOLATION = DWORD(0xc0000005) + EXCEPTION_ACCESS_VIOLATION = DWORD(0xc0000005'i32) EXCEPTION_CONTINUE_SEARCH = Long(0) type diff --git a/lib/windows/winlean.nim b/lib/windows/winlean.nim index 49d65c251e..75c0aeacfe 100644 --- a/lib/windows/winlean.nim +++ b/lib/windows/winlean.nim @@ -832,9 +832,9 @@ template hasOverlappedIoCompleted*(lpOverlapped): bool = (cast[uint](lpOverlapped.internal) != STATUS_PENDING) const - IOC_OUT* = 0x40000000 - IOC_IN* = 0x80000000 - IOC_WS2* = 0x08000000 + IOC_OUT* = 0x40000000'i32 + IOC_IN* = 0x80000000'i32 + IOC_WS2* = 0x08000000'i32 IOC_INOUT* = IOC_IN or IOC_OUT template WSAIORW*(x,y): untyped = (IOC_INOUT or x or y) diff --git a/tests/misc/tconv.nim b/tests/misc/tconv.nim new file mode 100644 index 0000000000..2384c3e9d7 --- /dev/null +++ b/tests/misc/tconv.nim @@ -0,0 +1,43 @@ +template reject(x) = + static: assert(not compiles(x)) + +reject: + const x = int8(300) + +reject: + const x = int64(NaN) + +type + R = range[0..10] + +reject: + const x = R(11) + +reject: + const x = R(11.0) + +reject: + const x = R(NaN) + +reject: + const x = R(Inf) + +type + FloatRange = range[0'f..10'f] + +reject: + const x = FloatRange(-1'f) + +reject: + const x = FloatRange(-1) + +reject: + const x = FloatRange(NaN) + +block: + const x = float32(NaN) + +type E = enum a, b, c + +reject: + const e = E(4)