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.

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

new {.disabled.} pragma #213

Closed
timotheecour opened this issue Apr 28, 2020 · 4 comments
Closed

new {.disabled.} pragma #213

timotheecour opened this issue Apr 28, 2020 · 4 comments

Comments

@timotheecour
Copy link
Member

timotheecour commented Apr 28, 2020

summary:

semantics of proc foo {.disabled.} = ...

  • declared(foo) should return false (unless declared(foo) was already true because of another symbol with same name); consequently, nim doc won't generate a corresponding entry, but maybe nimsuggest could list it as candidate showing it's disabled
  • a symbol foo {.disabled.} should not participate symbol lookup nor in overload resolution unless there is an error; in particular, it cannot cause a redefinition error

The only semantic difference between proc foo {.disabled.} = ... and when false: proc foo = ... is that it gives better error messages when lookup/sigmatch fails:

  • when lookup for an ident "foo" fails, instead of Error: undeclared identifier: 'foo' we show: Error: usage of 'foo' is {.disabled.} at mymodule.nim(12,1)
  • when sigmatch for overloaded "foo" fails, we add to the list of candidates in expected one of ... list the disabled symbol (and show the {.disabled.} pragma)

other use cases of {.disabled}

  • replace {.error.} by {.disabled.} in code disabled for for custom targets (js,nimscript), so we avoid import os + use of existsDir/dirExists/existsFile/fileExists/findExe in config.nims causes "ambiguous call' error Nim#14142 while keeping informative errors

  • use it inside the {.since: (1, 3).} pragma for nicer error messages without changing semantics of since; so now you'll get more informative errors like:
    `newSymbolX` is available starting 1.3.1, not 1.2.0

  • with when cond: {.push disabled}, we can conditionally disable all the remainder of the code in the same scope without having to re-indent under when not cond; this somewhat related to D's __EOF__ special token except it's during semantic, and is more useful (eg: scoped)

  • in particular, when used at module-level, this allows conditionally disabling all remainder of a module, which is needed for an RFC I'm preparing that will allow introducing user defined self-contained module metadata, and reduce the need for separate foo.nims files, magic testament spec blocks, magic exclusions in kochdocs, magic comments indicating this is only an include file etc. More on this soon.

  • (optional feature): disabled inside push/pop section
    this reduces the need for having to re-indent a whole section, eg:

# this line is not disabled for js
when defined(js): {.push("mylabel"): disabled.}
# this line is disabled for js
proc fun1(f: File) = ...
proc fun2() = ...
{.pop("mylabel").} # this pops what was pushed with the same label (mylabel); this prevents pop from being disabled! it's ignored if no matching push with the same label.
# this line is not disabled for js

implementation

  • seems straightforward
  • like error, it can also take an optional message string argument

why not use {.error.} instead for that?

semantics are different, {.error} procs participate in overload resolution and declared(foo) is true

why not change semantics of {.error.} to be like disabled instead?

that'd be a breaking change (eg: declared(foo) would turn from true to false), and IMO {.error} fullfils a different role than {.disabled}

why not use template disabled(body): untyped = discard instead?

@Araq
Copy link
Member

Araq commented Apr 28, 2020

Can you elaborate on this point

even if all these were fixed, more importantly, you'd loose all benefits wrt improved error messages; in particular when the disabled code is inside a {.push disabled} section and it may not be obvious at 1st glance that a symbol was disabled

As I understand the RFC the error messages would be incomprehensible with disabled too.

@mratsim
Copy link
Collaborator

mratsim commented Apr 28, 2020

This is interesting:

  • The reindent is quite annoying, especially when writing a very low-level library that needs to deal with Linux, Windows, MacOS, x86_32, x86_64, ARM32, ARM64, compilers, .... This happens in cryptography to deal with the various implementation of uint128 or in everything that needs OS and/or compiler support (multithreading primitives, IO primitives, ...)

One thing that I would add is that disabled is not taken into account in nimsuggest and nim docs.
The reason why is that you may be developing for one target which is disabled by default (CUDA, OpenCL).
My workaround in Arraymancer is to have different files and dispatch depending on docs/nimsuggest. This also allows me to avoid reindent (https://github.com/mratsim/Arraymancer/blob/fe896870f8a67f961a930f832af72354f32c3da2/src/tensor/tensor.nim#L73-L79)

when defined(cuda) or defined(nimdoc) or defined(nimsuggest):
  import ./tensor_cuda
  export tensor_cuda

when defined(opencl) or defined(nimdoc) or defined(nimsuggest):
  import ./tensor_opencl
  export tensor_opencl

Additionally, a disable message "This feature is disabled unless you explicitly compile with -d:cuda. Please refer to documentation at https///abc.efg" might also help library authors develop experimental features and making sure to point people at direct documentation (or bug report).

Furthermore, when managing a library function lifetime, we could smoothly transition from "live function -> deprecated -> function -> disabled function -> undeclared identifier". With the deprecated and disabled steps directly linking to the relevant changelog or mentioning the alternative.

@timotheecour
Copy link
Member Author

timotheecour commented Apr 28, 2020

@Araq

As I understand the RFC the error messages would be incomprehensible with disabled too.

Why? with {.disabled.} in the case of sigmatch/lookup error, can show the same information as {.error.} (which used to be super cryptic until nim-lang/Nim#13833 where I added where the symbol was declared); with symbol location, it should be easy for user to figure out why it was disabled (identical effort compared to {.error.})

with {.push disabled}, it would work the same; you'd get symbol location, and potentially (as enhancement) we could also report in errmsg the location of the enclosing {.push disabled}, but even without that it shouldn't be hard to grok. Note that os.nim doesn't push but uses pragma pragma so it's not relevant for current os.nim.

example1: {.disabled.}

# module foo
proc fun0*() = discard
proc fun1*() {.disabled.} = discard
# module main
from foo import fun1 # Error: proc fun1() [declared at pathto/foo.nim(2,1)] is {.disabled.}

import foo
fun1() # => Error (ditto)

import bar # which contains fun1()
fun1() # ok, no redefinition error

example2: {.push disabled.}

# module foo2
when defined js: {push("label_js_disable"): disabled.} # just to illustrate push/pop with labels
proc fun0*() = discard
proc fun1*() {.disabled.} = discard
{.pop("label_js_disable").}
# module main
import foo2
fun1() # Error: proc fun1() [declared at pathto/foo2.nim(3,1)] is {.disabled.} # (good enough)
fun1() # Error: proc fun1() [declared at pathto/foo2.nim(3,1)] is {.disabled.} at pathto/foo2.nim(1,1) # (doable too)

example3: non-nested labeled push/pop

  • that's again an optional part of RFC, but I think it's useful; the labeled-push/pop frees us from having to nest those attributes, which often isn't desirable as these are un-related
  • it also makes push/pop pair correspondence self documenting, unlike current situation which can be hard to grok:
{.push bar.}
lots of code
{.push foo.}
lots of code
{.pop.} # which push did I pop ? (especially since pop is optional)

@mratsim
yes, avoiding noisy re-indents is one of the intended goals; they're both inconvenient and cause large git diffs when a PR needs to add a large conditional.

One thing that I would add is that disabled is not taken into account in nimsuggest and nim docs.

that's implied by the declared(foo) is false rule, but ya, I'll clarify that; note that which target to generate docs for is a separate issue, and not hard to fix (eg: nim doc --backend:js, I'm introducing --backend in another PR for this and also for nim r --backend:js bar)

Additionally, a disable message "This ..."

of course; {.disabled.} can take an optional msg argument

Furthermore, when managing a library function lifetime, we could smoothly transition from "live function -> deprecated function -> disabled function -> undeclared identifier". With the deprecated and disabled steps directly linking to the relevant changelog or mentioning the alternative.

great point! good idea; and at the {.disabled.} stage we can get rid of proc body, so code is already simplified

@Araq
Copy link
Member

Araq commented Oct 28, 2020

The only semantic difference between proc foo {.disabled.} = ... and when false: proc foo = ... is that it gives better error messages when lookup/sigmatch fails:

Sophistry.

You can also use #[ ... ]# for a multi-line commenting out feature, no harmful re-indendentations required.

@Araq Araq closed this as completed Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants