Force experimental switch to be able to use call/dot operators

[metagn]

closes #13034

This changes the behavior of call and dot operators, they can now be defined without an experimental switch without erroring, but the switch must be turned on in order to use them. This means that before, an operator could be defined with one push experimental, and then to use it you did not need an experimental switch, but now it's the opposite.

This is going to break a fair amount of code, they will need to turn on the experimental switches like done in some old tests in this PR, but the errors should not be too uninformative and it's better to deal with this than silently having experimental operators available.

Currently breaks tests for:

• nimsl (test needs experimental dotOperators for swizzling, PR made)
• nimpy (nim tests need experimental dotOperators, CI only shows tpyfromnim.nim but tbuiltinpyfromnim.nim, nimfrompy.nim seem like they need it too, PR made)
• react (example app needs experimental callOperator, PR merged)

EDIT: refs callOperator, dotOperators (more searcheable)

[metagn]

nimpy, nimsl and react break with these changes

[timotheecour]

This is going to break a fair amount of code

that's a necessary evil to fix #13034; plus, experimental flags are explicitly not part of stability guarantee (my understanding of experimental flags is: the API can change but the feature is likely to remain available in some form or another)

can you list in PR description the list of broken packages, to make it easier to patch them?

options to make this PR green:

• disable broken packages with a comment # pending bug <link>
• pass --experimental:dotOperators in nimble command for those broken packages but this may have unintended effects
• introduce a -d:nimLegacySpecialOperators which would allow code to avoid a breaking change by passing this flag; it doesn't have to revert the full previous behavior, but maybe just what's needed to make those packages green; note that -d:nimLegacySpecialOperators may be less intrusive than passing --experimental:dotOperators on cmdline; maybe something like this (untested) ?

if callOperator in c.features:
  result = overloadedCallOpr(c, n)
=>
if callOperator in c.features or isDefined(c.config, "nimLegacySpecialOperators"):
  result = overloadedCallOpr(c, n)

(this could be passed in nimble command for such packages or, if that doesn't work, via nim-lang/RFCs#336)

[metagn]

I can see it can be disruptive for people to have to write the experimental switch every time, and the errors aren't immediately obvious, but as long as it's still an "experimental" feature I think it makes sense. Using a new define would be odd here, as one could just add the experimental switch instead of adding the define, though defines can "spread" to other modules unlike experimentals. I'm fine with a little wait until those packages are patched, if it takes too long then they can be commented out of the CI or --experimental can be passed through the command.

[timotheecour]

failing packages:

• thttpclient_ssl.nim, thttpclient_ssl_remotenetwork.nim fails on linux: genericDeepCopyAux SIGSEGV #16338 (flaky, probably unrelated to this PR)
  tests/misc/trunner_special.nim
  SIGSEGV: Illegal storage access. (Attempt to read from nil?)
  Error: execution of an external program failed: '/home/vsts/.cache/nim/thttpclient_ssl_remotenetwork_d/thttpclient_ssl_remotenetwork '
  2021-02-07T03:00:41.5600975Z /home/vsts/work/1/s/tests/misc/trunner_special.nim(22, 12): Check failed: ret == 0

• FAIL: nimpy c
  /home/runner/work/Nim/Nim/pkgstemp/nimpy/tests/tpyfromnim.nim(5, 13) Error: attempting to call undeclared routine: 'sum'

• FAIL: nimsl c
  /home/runner/work/Nim/Nim/pkgstemp/nimsl/test.nim(14, 18) Error: undeclared field: 'zw' for type nimsl.Vec4 [type declared in /home/runner/work/Nim/Nim/pkgstemp/nimsl/nimsl/nimsl.nim(28, 5)]

• FAIL: react c
  /home/runner/work/Nim/Nim/pkgstemp/react/example/app.nim(66, 19) Error: attempting to call routine: 'search'

@hlaaftana since it's only 3 packages, how about sending 3 PR's to unblock yours?

[yglukhov]

I might be missing something, but are you saying you're breaking nim 1.0, some existing code (likely all nimpy users and some nimsl users), to make their code uglier (add experimental pragma wherever needed), just to hide the real issue (which was always there) where a generic myCall[int](1) non-idiomatic call doesn't work?

[timotheecour]

@yglukhov what do you suggest for #13034 ?

[yglukhov]

I claim that dotCalls and dotFields is a useful feature to the extent that many people using it don't find it that unstable. Please prove me wrong. But if we all agree on that, I'd focus the efforts on moving the feature out of its experimental status, by fixing the few remaining issues. It's discouraging to see how much effort you guys are wasting to do the opposite.

Regarding the #13034, I'd just fix the spec and call it a day.

[metagn]

The changes to the experimental switches themselves required no "effort". The point is that it's more intuitive this way, not that people should go through more trouble to use these operators. A lot of people have most likely been adding the experimental switch to their code that uses the operators already (via pragma or command), because that's what's intuitive.

I don't know the compiler well enough to find out what causes #6981, and there are bugs like #7777, #15607 which I also don't understand. I can't really deal with pondering the question of whether this feature should be experimental at all, because I personally can't measure the intensity of these specific bugs and predict the timeframe of when they will be fixed or how much they matter or whether or not there are worse bugs. I made this PR despite that, you can blame me for "discouraging" you but I won't care if the operators are made non-experimental and my change isn't made. I can't stop myself from working on remotely potentially helpful changes, but there's seriously no reason for anyone to be "discouraged".

Beyond that, it is a mistake to have both the experimental switch and the new tests/docs in the same PR, I can make a new one for the new tests/docs.

[timotheecour]

what causes #6981, and there are bugs like #7777, #15607 which I also don't understand.

and #13063 also which is a pervasive issue affecting invalid field accesses once you import a module like jsffi or wrapnils; hopefully this is fixable though.

[yglukhov]

It just occurred to me that dotOperators and callOperators require different experimental pragmas, so I suggest we discuss them as separate features.

#6981 is actually about the callOperator, and is somewhat concerning since the feature affects otherwise unrelated code. I myself hit quite a few problems with callOperators previously, so I prefer to avoid them now.

On the other hand #7777, #15607 are about dotOperators, but the issues happen within intentional use of those. #13063 is a nuisance indeed, but it's about an error message, as the code is invalid anyway.
Otherwise I see no issues of a kind where dotOperators would introduce a serious problem in the code that doesn't ask for it :). Which makes me think that dotOperators are closer to stable than experimental, and only a few minor issues remain.

Therefore I'm asking to reconsider the breakage of dotOperators for little to no reason. For "intuitive way" sake the dotOperators should be working with respective experimental pragma. For backwards compatibility and the fact that one day they will become stable, they should be working without the pragma :).

I made this PR despite that, you can blame me for "discouraging" you

@hlaaftana no offense, "discouraging" might not be the right word, maybe "sad" would be a better one. Regardless, I value your contributions, please continue, and don't let my words discourage you 🙏

[metagn]

I can agree there is not much immediate advantage to these changes. I don't know which is more suitable, perhaps tag as "Postponed", or close so that it may be reopened again, when there is an objective reason to cave in to stricter experimental switches, but it's unlikely to find this reason. Or maybe the new behavior should be opt-in, which would be odd and I don't know if it would prevent only user error and not bugs.

[yglukhov]

Sounds good to me

Or maybe the new behavior should be opt-in

Can you clarify what you mean by that?

[metagn]

Can you clarify what you mean by that?

A define or something where if callOperator in c.features: and the like becomes if callOperator in c.features or not isDefined(c.config, "nimForceSpecialOpsSwitch"):. or something. I think some people might benefit from it.

[Araq]

These experimental operators should be removed from Nim altogether, IMHO. I completely dislike the idea that x.foo can fail at runtime because it's convered to x["foo"]. If x["foo"] is too verbose, there are other ways of using shortcuts like x!foo where ! is a macro operator.

[yglukhov]

there are other ways of using shortcuts like x!foo where ! is a macro operator

Please tell me more. Just tried ! and it seems to be of the wrong precedence/associativity. What operators can be used instead of . for field access and calls? TBH I like the idea of using a different dot-like operator, even though I don't see anything wrong with x.foo "failing at runtime".

[metagn]

The highest operators in precedence would be like x|foo, x\foo, x$foo. You can also work with something like \foo.x. I think these are not necessarily useful ideas given the original goal seems to have been to appeal to people familiar with Ruby method_missing and Python __getattr__ etc. I think it still has appeal in NimScript or DSLs like nimsl (though having to use untyped for just a subset of strings might not be great). Scripting language wrappers like nimpy don't necessarily need dot operators to exist, you could easily make them have to use vm.pyGetProperty(obj, "propertyname") or something verbose like that, it's just syntactic sugar in their case. In a Nim "strict" mode maybe they could be disallowed. If there was a vote on it I'd say many people would vote against these operators being removed.

[timotheecour]

note that i was able to forgo the use of dotOperators in std/wrapnils (see #16996), which leaves us with the use of dotOperators in jsffi/nimpy + friends, where you need some kind of dynamic field access (note: in D, that's essentially opDispatch).

.? could be a perfect fit for that, since it visually differs from . (static vs dynamic field access) and the ? reminds that the field is potentially invalid (dynamic nature); the problem is the operator precendence.

proposal

change precedence rules so that .? and . and .? have same associativity/precedence. Currently this isn't the case as shown here:

when true:
  import macros
  macro deb(a: untyped): untyped =
    echo a.lispRepr
  deb a.b.c # parsed as: (a.b) . c
  deb a.b.?c # parsed as: (a.b) .? c
  deb a.?b.c # parsed as: a .? (b.c)

(DotExpr (DotExpr (Ident "a") (Ident "b")) (Ident "c"))
(Infix (Ident ".?") (DotExpr (Ident "a") (Ident "b")) (Ident "c"))
(Infix (Ident ".?") (Ident "a") (DotExpr (Ident "b") (Ident "c")))

under this rule, a.?b.c would instead parse as: (a.?b).c, and .? would then be a valid replacement for user-defined ., in particular for jsffi or nimpy, without having to mess around with builtin .:

let j = (a: 1, b: 2).toJson
j.?bar.?baz

there are 2 reasonable ways to do that:

proposal A1

we defined dot like operators as operators starting with ., but excluding operators starting with .. like ..<, ..^
dot like operators shall have same precedence as .

proposal A2

we defined question-mark operators as operators ending with ?
question-mark operators get the same precedence as the operator stripped from the trailing ?

so that .? would have same precendence as .

note

that leaves with remaining cases where dotOperators could still be useful, namely type punning, where we change an API transparently (more so than via a template field(a, b): untyped which isn't quite a 1:1 replacement; this is more rare than the use case in jsffi/nimpy+friends and a separate discussion.

EDIT: see RFC nim-lang/RFCs#341

[Araq]

I like your "proposal A1" much better. We should implement it and see what it breaks. :-)

[metagn]

I think these proposals warrant RFCs. They can be treated independently as they are not mutually exclusive.

These experimental operators should be removed from Nim altogether

As breaking as it already is, this PR would be a good start for this (if the feature is removed, defining the operators should be a nop, it's really only at callsites where the feature can cause problems). Even though I don't support removing the operators, I think it's still worth seeing what people think; maybe say deprecate for now instead of remove.

change precedence rules so that .? and . and .? have same associativity/precedence

I guess this is pretty uncontroversial (I actually thought you couldn't start operators with ., only ..), but it would help keep track of the idea to have an RFC on it.

About PR: I don't see a need for another commit for now, will let merge conflicts pile up until and if CI can be expected to pass

[timotheecour]

=> RFC nim-lang/RFCs#341

[saem]

I'll save most of it for the RFC (if I can muster up the courage), but I think this doesn't go far enough and dot operators need a much bigger rethink.

[metagn]

Closing, doesn't seem this will improve anyone's experience at the time.