one definition rule for `defined` symbols: `defined(foo)` requires `declare(foo)`

timotheecour commented 3 years ago

This proposal will make defined symbols self documenting (using doc comments on declare(foo))
It provides a solution to the problem of clashes where 2 libraries use defined(foo) with 2 different meanings.
It also helps by giving a warning when using a mistyped or nonexisting defined symbol: when defined(rellease) will now give a WarnDefinedUndeclared under this proposal.
it also helps for migrating code, when a defined symbol is renamed from foo to bar (or when foo is deprecated), we currently have no easy way to encourage users to change their code; under this proposal this becomes trivial

proposal

This proposal adds type safety to defined statements, requiring each use of defined(foo) to be preceded by a unique declare(foo) statement, in other words, adding "one definition rule" to defined statements.

when defined(foo): discard # warning: defined(foo) undeclared [WarnDefinedUndeclared]
{.undef(foo).} # ditto
{.define(foo).} # ditto

declare foo # this can be defined inside a module, which must be imported before defined(foo) is used
when defined(foo): discard # ok

import bar

# bar.nim
declare foo # warning: `declare foo` already declared in main.nim(4,1) [WarnDefinedRedeclared]

note that declare(foo) doesn't define foo, unlike {.define(foo).} or -d:foo on cmdline; it merely prevents getting a warning when using when defined(foo) or {.undef(foo).} or {.define(foo).}

for legacy defines (eg: -d:danger, -d:release etc), these would be declared in system.nim as follows:

# in system.nim
declare danger
declare release
# etc

for defines in 3rd party packages, it's the same:

# in cligen.nim
declare cligenFoo

independently of this, for the scoping, we'd use: https://github.com/nim-lang/RFCs/issues/181 to allow namespacing, eg:
```
# in cligen.nim
declare cligen.foo
# in some other main.nim
import cligen
when defined(cligen.foo): discard
```
so this proposal is complimentary to https://github.com/nim-lang/RFCs/issues/181 (ie both can be implemented and make sense together).

declared symbols would be exposed via an API and via docgen

currently we rely on manually generated list of defined symbols eg: https://nim-lang.github.io/Nim/nimc.html#additional-compilation-switches

Under this proposal we'd be able to have the declared symbols shown in docs in a new "declared defined symbols" category.

# in system.nim
declare danger ## Turns off all runtime checks and turns on the optimizer.

=> docgen would pickup the doc comment, as it would for other symbols

macros.nim would also allow listing all declared symbols for reflection purposes.

EDIT: note: `declare` can be bikeshedded

an alternative could be declareDefine, or {.booldefine.} as explained in https://github.com/nim-lang/RFCs/issues/269#issuecomment-711445106

const foo {.booldefine.} = false ## some comment

with same ODR rules as defined in this RFC.

Clyybber commented 3 years ago

Instead we can make docgen recognize top-level when statements that look like this:

when defined(foo):
  ## Documentation for foo

timotheecour commented 3 years ago

Instead we can make docgen recognize top-level when statements that look like this:

that doesn't help with defined symbol clashes. defined(foo) can be used multiple times (and in different modules), but should have a single corresponding unique declare foo ## some doc comment in scope (possible appearing in some imported module)

Clyybber commented 3 years ago

The same pattern could be detected and warned about with my proposal.

This RFC is only really worthwhile combined with #181 IMO, but then again nothing prevents you from prepending your module/project name to your defines as we do already.

So the aforementioned pattern combined with using a prefix already solves all problems that this RFC and #181 solve and doesn't require a new language construct (declare is also a bad name because there is system.declared which is not related (could be renamed to inScope/inCurrentScope tho))

EDIT: One advantage that this RFC has is that you will get a warning on typos like defined(foob)

timotheecour commented 3 years ago

your proposal gives a new meaning to something that already has a (useful) meaning, eg:

when defined(js):
  ## doc comment specific to js

and furthermore is less searchable; using a dedicated API for that is better than conflating pre-existing syntax.

declare is also a bad name because there is system.declared which is not related

happy to bikeshed alternative names

EDIT: One thing this RFC improves is that you will get a warning on typos like defined(foob)

yes, this is in fact common, thanks for reminding me to add that, it was also a motivating factor for this RFC; defined(foob) indeed doesn't give you any warning when you've either mistyped it, or when that defined symbol was removed

prefix already solves all problems

it's not enforced, and is regularly abused including in nim repo, despite the fact we caution against it, see https://nim-lang.github.io/Nim/contributing.html#best-practices. Furthermore it doesn't solve all problems, eg it doesn't allow you to list/extract doc comments for those defined symbols, and doesn't help with prefix issue mentioned in #181

Clyybber commented 3 years ago

Another already existing functionality that provides the same advantages as this RFC, is {.booldefine.} and co. (although they don't allow checking for otherproject.foo, but this can be allowed via something similar to wrapnils.maybe that checks that otherproject exists and only then if it exists checks otherproject.foo; we could extend defined for this)

We could promote their usage and allow -d:bar.foo for them.

timotheecour commented 3 years ago

good to point out booldefine. booldefine currently doesn't give any warning/error for redefinition, eg:

# t11150b.nim
const foo1 {.booldefine.} = false

# main.nim
import t11150b
const foo1 {.booldefine.} = false

IMO it should for same reasons as provided in this RFC. I'd be ok with the following modification to this RFC:

replace declare foo by const foo {.booldefine.} = false
everything else in this RFC stays the same (in particular ODR still applies, and generate warnings when violated) this would have a benefit from reusing docgen for defined symbols. Note that for system.nim, to avoid polluting scope with release + friends we can do:
```
# in system.nim
import std/system_defines
# in system_defines.nim
const
release {.booldefine.} = false ## doc comment
danger {.booldefine.} = false ## doc comment
```

note that release doesn't need to be exported for its defined effect to be global (defined symbols are global). No inconsistency here.

Clyybber commented 3 years ago

We can put

const release {.booldefine.} = false

directly into system since the const itself wouldn't be exported and we can make defined also pick up private symbols that are {.booldefine.}, so that defined(release) will still work (no need to specify system.release here, because system should be special)

timotheecour commented 3 years ago

We can put [...] directly into system since the const itself wouldn't be exported and we can make defined also pick up private symbols that are {.booldefine.}

yes, no need for import std/system_defines indeed.

so that defined(release) will still work (no need to specify system.release here, because system should be special)

that I don't understand. this RFC wouldn't make system special (which is often a bad idea).

Here's what I have in mind, please suggest if you have a better idea:

# in system.nim
const release {.booldefine.} = false
when defined(release): discard # ok

# under #181
when defined(system.release): discard # error: system.release not booldefined
const foo {.booldefine: "nim.foo".} = false
when defined(nim.foo): discard # ok
when defined(foo): discard # error: foo not booldefined

Clyybber commented 3 years ago

Ah yeah, I was thinking of automatically scoping {.booldefines.} to their module and then special casing system, but what you propose works too.

Araq commented 3 years ago

I use this idiom instead:


const
  debugOrc = defined(nimDebugOrc)

when debugOrc: ...

Typo-safe, uses an existing language feature that works well with Nim's scoping rules.

Clyybber commented 3 years ago

Thinking about it, {.booldefine.} could be made a macro that turns:

const release {.booldefine.}

into

declare release
const release = defined(release)

which would also map better to .cfg files (@declare foo)

timotheecour commented 3 years ago

const debugOrc = defined(nimDebugOrc)
when debugOrc: ...

this doesn't work.

doesn't work with {.undef.}, {.define.} (which are very handy in some use cases)
it's not typo safe, no warning is issued if you mistype defined(nimDebugOrc)
it doesn't address the problem of multiple libraries defining the same symbol with their own meaning

Araq commented 3 years ago

(Of course it does work, I use it successfully.)

it's not typo safe, no warning is issued if you mistype defined(nimDebugOrc)

That's true but if we address this problem, we should also catch mistyped --defines on the command line. Which can be done rather easily: Remember all the defined(x) calls in the compilation unit and issue a warning afterwards if your --define didn't come up.

it doesn't address the problem of multiple libraries defining the same symbol with their own meaning

Libraries shouldn't use --define switches anyway, every switch creates a unique library version.

timotheecour commented 3 years ago

doesn't work with {.undef.}, {.define.} (which are very handy in some use cases)

(Of course it does work, I use it successfully.)

@Araq it doesn't work reliably if you have {.undef.}, {.define.} anywhere in your code:

const foo {.booldefine.} = false
proc main()=
  echo (foo, defined(foo))
  {.define(foo).}
  echo (foo, defined(foo))
static: main()

(true, false) # foo != defined(foo) (true, true)

And yes, those flags {.undef.}, {.define.} are very useful, not a corner use case.

That's true but if we address this problem, we should also catch mistyped --defines on the command line. Which can be done rather easily: Remember all the defined(x) calls in the compilation unit and issue a warning afterwards if your --define didn't come up.

yes, I thought about adding that too in the RFC, definitely doable and should be done.

Libraries shouldn't use --define switches anyway, every switch creates a unique library version.

nim relies on it so much because nothing else fits that use case yet (open config flag), so it's not surprising that other packages will too, following existing practices.

It's broken though and will only get worse as more packages are added in the wild, which is why I'm trying to fix it in #269 and #181 to give it sane, scoped, typesafe semantics.

The fact is that many nimble packages use their own defined, and very few prefix their defined symbols with the package name.

Can you guess which defined comes from which package without looking at the title?

## arraymancer
defined(nnpack)
defined(no_lapack)
defined(opencl)
defined(openmp)
defined(native)
defined(cuda)
defined(cudnn)
defined(blis)

## cligen
defined(cgCfgToml)
defined(cligenSingle)
defined(debugCfToCL)
defined(debugEnvToCL)
defined(debugMergeParams)
defined(llvm_gcc)
defined(printDispatch)
defined(printDispatchDG)
defined(printDispatchMulti)
defined(printDispatchMultiGen)
defined(printIDGen)
defined(printInit)
defined(testDropPriv)
defined(versionGit)
defined(versionNimble)
defined(versionShort)

## nimble
defined(nimble)
defined(nimdistros) # not in nim!
defined(nimscript) # not in nim even though a common prefix of nim and nimble
defined(passNimIsWorking) # not in nim

## laser
defined(fast_math)
defined(fastmath)
defined(march_native)
defined(openmp)

## freeimage
defined(PLUGINS)
defined(lnkStatic)

## karax
defined(karaxDebug)
defined(debugKaraxDsl)
defined(profileKarax)
defined(stats)

## inim
defined(NOTTYCHECK)
defined(NoColor)
defined(withTools)

## fusion
defined(testing)

## jester
defined(useStdLib)

## anonimongo
defined(oldto)
defined(verbose)
defined(verifypeer)
defined(gridEnsured)
defined(nomongod)
defined(uri)

etc...

I have 324 unique defined symbols in a small ~/.nimble_tmp/pkgs folder, and no idea how many clash, or which belongs to which, or which are typos like defined(emcsripten) in jsbind.nim /cc @yglukhov

disruptek commented 3 years ago

I believe we already have guidelines suggesting that authors name their defines according to their package, eg. frostyMagic, frostyDebug, frostySorted or skiplistsChecks, skiplistsGrowth.

I've been using this style since I started writing Nim. Coupled as it is with @Araq's technique above, it works fine.

If defines are a problem, well, this isn't the fix. Scoping them just sweeps the issue under the rug and makes it even easier to fragment libraries and harder for them to share defines or inspect each other.

Araq commented 3 years ago

We already have "scoped" defines, they are called const and we have .booldefine so that they can be set via the command line. We can patch modules to use these mechanisms, your solution of making them adopt a declare statement is just as much work but more costly as it adds yet another compiler/language feature.

However we should ensure that --define:module.symbol=value works.

Araq commented 3 years ago

Closing in favor of https://github.com/nim-lang/RFCs/issues/181

nim-lang / RFCs