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

fix RFC #341: dot-like operators are now parsed with same precedence as . #18711

Merged
merged 9 commits into from
Aug 25, 2021

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Aug 19, 2021

fix RFC nim-lang/RFCs#341

in particular, this allows having a nice dot-like syntax for dynamic fields (eg jsffi, json, nimpy etc) without messing with ., thus avoiding the associated downsides:

  • bad error msgs when something doesn't match either a field nor a dot operator
  • . requires {.experimental: "dotOperators".}

example

this now works:

when true:
  import std/json
  template `.?`(a: JsonNode, b: untyped{ident}): JsonNode =
    a[astToStr(b)]
  let j = %*{"a1": {"a2": 10}}
  doAssert j.?a1.?a2.getInt == 10

future work

consider also supporting dot-like assignment operators:

when true:
  import std/json
  template `.=`(a: JsonNode, b: untyped{ident}, c: untyped): untyped =
    a[astToStr(b)] = c
  template `.?=`(a: JsonNode, b: untyped{ident}, c: untyped): untyped =
    a[astToStr(b)] = c
  var j = %*{"a1": {"a2": 10}}
  echo j
  j.a1 = newJNull() # ok
  # j.?a1 = newJNull() # not yet ok
  echo j

@timotheecour timotheecour added the Ready For Review (please take another look): ready for next review round label Aug 19, 2021
@timotheecour timotheecour force-pushed the pr_rfc_341_dotop_precedence branch from 2429087 to 282d242 Compare August 19, 2021 16:07
Copy link
Member Author

@timotheecour timotheecour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL

@Araq
Copy link
Member

Araq commented Aug 19, 2021

Did you consider this implementation instead:

  • The lexer produces a tkDotLikeOpr token kind.
  • The parser treats tkDotLikeOpr just like tkDot?

Seems cleaner to me as it's then by construction that dot and dot-like operators are handled identically.

@timotheecour
Copy link
Member Author

timotheecour commented Aug 19, 2021

Did you consider this implementation instead:

I did, and decided on doing it the way it's done in this PR instead; introducing tkDotLikeOpr would be a much more invasive change, requiring changes to docgen, renderer, parser, layouter, lexer ... for no tangible benefits. I prefer the simplest approach here, with a localized handling of dot-like operators.

@Araq
Copy link
Member

Araq commented Aug 20, 2021

Still needs to be opt-in for version 1.x

@timotheecour timotheecour force-pushed the pr_rfc_341_dotop_precedence branch from 7b5977f to aa856ba Compare August 20, 2021 18:13
@timotheecour
Copy link
Member Author

timotheecour commented Aug 20, 2021

PTAL

Still needs to be opt-in for version 1.x

1.x doesn't make sense (code silos etc), but I'm ok with making it opt-in for 1.6. I've now introduced -d:nimPreviewDotLikeOps as follows:

  • 1.6 will default to nimPreviewDotLikeOps undefined
  • devel (starting with this PR) will default to nimPreviewDotLikeOps defined
  • warnDotLikeOps will be issued when a dot-like operator is used and nimPreviewDotLikeOps is undefined

@Araq
Copy link
Member

Araq commented Aug 20, 2021

1.x doesn't make sense (code silos etc), but I'm ok with making it opt-in for 1.6.

That's what I meant anyway, thanks.

@arnetheduck
Copy link
Contributor

devel (starting with this PR) will default to nimPreviewDotLikeOps defined

this isn't really a sustainable strategy since then, devel will be testing a different setup than what will be released - what makes sense is that when the first release with the disabled feature out, devel is switched - this allows for appropriate testing when the code without the flag enabled has been through one release cycle.

@arnetheduck
Copy link
Contributor

more concretely, that means that the feature lives in devel but is disabled. then 1.6 is released with the feature disabled. then devel is switched to enabled the flag. then a decision is made whether to enable the flag in 1.8+.

@timotheecour
Copy link
Member Author

this isn't really a sustainable strategy since then, devel will be testing a different setup than what will be released

it is a sustainable strategy. Nim users that want the latest features and don't want to wait another 6 months/1 year for next release can track devel, nim users that want a stable version can track the latest release, everything works.

CI for devel ensures ecosystem works with latest features, CI for version-1-4 and yet to be branched version-1-6 ensures that ecosystem works with tip of 1.4 and 1.6. No need to wait for 1.8 which is potentially a year away from now.

@arnetheduck
Copy link
Contributor

. Nim users that want the latest features and don't want to wait another 6 months/1 year for next release can track devel, nim users that want a stable version can track the latest release, everything works.

The users that don't want to wait have a trivial way to do that: include a flag - the whole point of introducing things gradually is to not create a situations where code ends up accidtentally depending on a new behavior - this is not so much about end users but libraries in between which often will not support both versions of every flag someone with an itch to scratch manages to throw into stdlib.

CI for devel using the experimental versions of code means that any usage of the unexperimental, stable version of code gets less testing which is quite nuts - the aim of an opt-in flag is to provide stability and overlapping adjustment time - for that reason, the stable version is the more important one to maintain, in CI and testing above all.

IF CI should be testing the new behavior, -d:enableAllBreakingChanges is more reasonable to run as a separate CI run.

@arnetheduck
Copy link
Contributor

This is extra important for library developers: they should be able to run nim devel to prepare their libraries for the next stable version of Nim without having to remember all flags that were added to the std lib to maintain some semblance of sane backwards compatibility - this enables them to release a stable version of their library together with the next stable nim release.

@timotheecour
Copy link
Member Author

there is so far zero evidence that this change breaks any code, in particular no code in important_packages; your general approach judging from your comments in PRs, issues, RFCs seems to be "let's change nothing", and ignores the reason for those proposed changes in the first place. No-one is forcing you to upgrade from a prior stable release if you care about stability above anything else.

nim-lang/RFCs#341 was already marked as accepted and had AFAIK support from everyone who cared to comment. The only thing you achieve by delaying introducing a preview flag as default is make it harder for libraries and user code to rely on this feature, and creates more incompatibilities down the road by persisting language dialects.

1.6 having nimPreviewDotLikeOps as off by default and devel as on by default is the most reasonable compromise here.

# devel gets bugfixes and some preview flags right away; next release gets bugfixes via a preview flag.
# these can be overridden using user/project/cmdline flags using `switch("undef", "nimPreviewX")`
# other `nimPreview` switches can go here, as needed.
switch("define", "nimPreviewDotLikeOps")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should instead have a todo for 1.7.x where we go through a list of preview options to consciously sanctionize new features we want to include for 1.7.x. The form of automization that you do here seems not wise.

Copy link
Member Author

@timotheecour timotheecour Aug 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the RFC was marked as accepted so it seems fair to turn this on in devel (except for 1.6); I can change the surrounding comment if needed but the comment as written doesn't imply that preview flags get turned on automatically, it says:

some preview flags and can go here, as needed.

which clearly states that these are turned on on a case by case basis.
Waiting for 1.8 to turn this particular feature on serves no benefit, it only makes it harder to use this feature in library code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-d:enablePreview18 sounds like a reasonable catch-all

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do as I said, I can only repeat my argument. We don't automate feature activations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done for the sake of moving on with this PR, but I still don't know what you mean by automate feature activations as I explained above; there's nothing automatic about hand-selected preview flags being turned on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we ensure it is enabled after Nim 1.6 is released? What if it is forgotten?

this sounds a bit like a non-problem - ie if it's forgotten, the feature is clearly not important or impactful enough that somebody cares/bothers, and one might want to reconsider its utility.

the other thing to consider is that the unit test suite should run with and without the flag, at least a subset of the tests where the old and new versions of the behaviour are touched / stressed: the way to do this is to run the targeted subset with the feature enabled while the feature is disabled by default, then run a targeted subset of tests for the old behaviour once the flag is stripped.

This also brings us back to a critical point: the more care is taken with backwards compatibility, the fewer problems of this nature there are and the easier it is to produce a new release - the complexity of introducing the change is merely being aligned with the effect it will have on existing code.

Copy link
Member

@ringabout ringabout Aug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this sounds a bit like a non-problem - ie if it's forgotten, the feature is clearly not important or impactful enough that somebody cares/bothers, and one might want to reconsider its utility.

It may be true.

IMO Nim language is a small community and only few people(araq and miran, other maintainers may leave Nim at any time) maintain it. People who like this feature may not know when and whether to change the flag. And people who proposal the feature may leave Nim.

So it is nice to make maintenance easier or talk about how to make it easier.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what would be really interesting is a warning that detects the difference in parsing and warns the user (this would be implementable in devel/next release already)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what would be really interesting is a warning that detects the difference in parsing and warns the user (this would be implementable in devel/next release already)

see warnDotLikeOps

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=> #18747

@arnetheduck
Copy link
Contributor

in particular no code in important_packages

aka "the unit test suite proves that there are no bugs in the code!", equally inaccurate though (see issue tracker)

"let's change nothing",

let's change things in a backwards compatible way, because each breaking change invalidates existing Nim code and requires the language and community to reboot for every crusade someone is on - this is difficult to work with.

When all options have been exhausted, introduce breaking changes over a period of time, because in a complex graph of dependencies, it's not possible to upgrade everything at once. I understand not everyone has visibility into this problem - it is, however, real for anyone with a significant codebase in nim.

To manage that, dogfooding the changes is a critical step - often, a better solution can be found than the initial one. devel should ideally be in a release-ready state most of the time so as to eventually allow to shorten the release cycles - this would also solve the time-to-market issues - this means that if a release is going to be made with the option off, that's what devel should be doing as well because that's the next reality that library and application developers need to be getting ready for, most urgently.

When developing libraries, developers that want to take advantage of the new flag or feature should be made aware that a flag will be needed in the next release - by having to specify the flag themselves, they also know exactly what users will experience when using their libraries, making it easier for them to write code in such a way that it supports code both with and without the flag, instead of waking up on release day to a Nim that works in a different way.

@timotheecour
Copy link
Member Author

timotheecour commented Aug 23, 2021

It's the same old tiring generic rant for every proposed change; if we'd follow your policy, nim would be in a much worse situation today, with tons of flags causing an exponential number of mutually incompatible language dialects for backward compatibility's sake (regardless of whether any actual code relied on old behavior), very slow progress, cemented bugs left for posterity ("just because some hypothetical code might rely on the bug", and at the expense of all other current and future users) and essentially a frozen 1.0 feature set with early design flaws baked in.

Any change is a potential breaking change for some use case, this shouldn't be a reason to stop nim development. A very large number of PR's since 1.0 would fail your stability test.

Just track a stable release number if you don't want any new features/bugfixes. This PR went through the RFC process and the RFC was marked as accepted, with large support.

@arnetheduck
Copy link
Contributor

arnetheduck commented Aug 24, 2021

It is tiring indeed to have to baby sit changes like these - the reason we do it is because we have a large working codebase based on Nim with a large team of developers actively using Nim every day and solving problems with it.

Your arguments are often of the fear and and uncertainty kind: if we don't change X, the resulting Nim will not be usable or maintainable - both are false and disproven by our projects - it's perfectly fine to use Nim in a real project today to create high quality code, in spite of the minor cosmetic details that the breaking changes often apply to - if's also perfectly possible to introduce many of these changes in a backwards-compatible way, but you consisently seem to ignore or fail to see this point - hence we end up in this situation where the same arguments need to be repeated - not for your benefit, but for the community such that they see that they can trust Nim not to pull the rug from under their project, should they choose to invest in it.

@arnetheduck
Copy link
Contributor

arnetheduck commented Aug 24, 2021

Any change is a potential breaking change for some use case, this shouldn't be a reason to stop nim development. A very large number of PR's since 1.0 would fail your stability test.

As a thought experiment, try stretching your imagination a bit and think of plausible answers to the following question: why do you think we're here pointing out that the model where Nim keeps breaking things is problematic?

@timotheecour timotheecour force-pushed the pr_rfc_341_dotop_precedence branch from aa856ba to 11cbd8a Compare August 24, 2021 18:34
@Araq Araq merged commit 3aa16c1 into nim-lang:devel Aug 25, 2021
@timotheecour timotheecour deleted the pr_rfc_341_dotop_precedence branch August 25, 2021 05:37
timotheecour added a commit to timotheecour/Nim that referenced this pull request Aug 25, 2021
timotheecour added a commit to timotheecour/Nim that referenced this pull request Aug 25, 2021
Araq pushed a commit that referenced this pull request Aug 26, 2021
* followup #18711 cleanup unused grammar rules

* make tools/grammar_nanny.nim report unused terminals

* revert removal of some grammar comments
PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
…cedence as `.` (nim-lang#18711)

* fix RFC nim-lang#341: dot-like operators are now parsed with same precedence as `.`

* fixup

* [skip ci] address comment in changelog

* address comment

* update grammmar

* add manual entry

* fixup

* -d:nimPreviewDotLikeOps

* address comment to unblock PR: move nimPreviewDotLikeOps out of config/config.nims
PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
nim-lang#18746)

* followup nim-lang#18711 cleanup unused grammar rules

* make tools/grammar_nanny.nim report unused terminals

* revert removal of some grammar comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review (please take another look): ready for next review round
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants