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

API renames must be backported, new {.deprecatedAlias.} syntax #214

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

API renames must be backported, new {.deprecatedAlias.} syntax #214

timotheecour opened this issue Apr 28, 2020 · 7 comments

Comments

@timotheecour
Copy link
Member

timotheecour commented Apr 28, 2020

nim-lang/Nim#13908 introduced a wide-ranging rename of exceptions to better reflect exception hierarchy, eg AssertionError=> AssertionDefect. While this was a good change, it creates a problem for backward compatibility; the current recommendation in nim-lang/Nim#13908 (comment) was to not backport it.

however this would pretty much guarantee a language drift and a not-so-slow death for 1.0 and 1.2: user code and third party code (or future packages) would face the impossible dilemma of either:

  • live with the old names forever (and cause deprecation warnings in devel), which negates the point of Error -> Defect for defects Nim#13908
  • litter their code with since (not practical, this would be a huge wasted effort)
  • drop support for 1.0 and 1.2 (the most likely outcome, because it's the easiest)

proposal

  • API renames should be backported to all supported nim versions (tips of 1.0.X and 1.2.X, ie HEAD of branches version-1-0 and version-1-2)
  • the backport for a rename FooOld=>FooNew doesn't need to change all the code using FooOld (that's bonus and can happen over time) but it needs to introduce the new alias FooNew
  • the aliases should be via the proposed {.deprecatedAlias.} syntax (or at least by the nimfix-era syntax: {.deprecated: [FooOld: FooNew].}), not via type FooOld* {.deprecated.} = FooNew was was done in Error -> Defect for defects Nim#13908 see the reason in section below.

benefits

  • the backport is minimal (only adds 1 pragma {.deprecatedAlias: [FooOld].} per renamed declaration)
  • 1.0.X (and 1.2.X, distinction irrelevant for this discussion) stays usable for a long time, and isn't affected by packages that start using the new aliases
  • users of 1.0.X don't need to change anything in their code besides upgrading to the latest patch release, eg 1.0.8 (which is designed to be a non-breaking change), or (if they can't wait for 1.0.8), try their luck with the tip of 1.0 branch
  • the deprecation warning helps users migrate their code to devel

bonus1: selective deprecation warning: --warning:deprecated:bar:off

The only effect on users of 1.0 branch when they upgrade to 1.0.8 is a deprecation warning (Warning: use FooNew instead; FooOld is deprecated). While they can use --warning:deprecated:off to silence it, it's a bit blunt and goes against strategies of "gradual migration"; so we could introduce (and backport) a simple way to target a specific deprecation warning, for example:

--warning:deprecated:off # turn off all deprecation warnings
--warning:deprecated:bar:off # turn off only deprecation warnings for warning called `bar`

for the API renaming, we could simply use --warning:deprecated:FooOld:off for simplicity but that doesn't take into account cases where FooOld would exist in different modules (or even different packages) since --warning:deprecated:FooOld:off has global scope; so instead we can simply introduce (and backport) this syntax: {. deprecatedAlias(bar): FooOld.} where bar is the name we give to this deprecation warning (in our case, it could be called pr13908 or errorVsDefect)

benefits: this allows grouping all related API renames under 1 deprecation name, so that users can use a single flag in their home config.nims: --warning:deprecated:errorVsDefect:off

bonus2: revive nimfix

  • nimfix (https://nim-lang.org/0.17.2/nimfix.html) was super useful in its time (migration to 0.10.0 required lots of renames due to the new improved style insensitivity rule IIRC) and there's no reason it can't be revived. Tooling is the best way to upgrade code, and should easily handle renames like {. deprecatedAlias(bar): FooOld.} {.deprecated(bar): [FooOld: FooNew].} with minimal (or probably 0) "manual fixups" needed.

edge case: partial API renames for overloaded symbols

this is an edge case, and (IIUC) is the reason you get Warning: the .deprecated pragma is unreliable for routines [User] when applying {.deprecated: [fooOld: fooNew].} to routines: when an API rename only renames some overloads of fooOld, {.deprecated: [fooOld: fooNew].} cannot work (would give: Error: redefinition of 'funOld').
That's one of the reasons for the proposed {.deprecatedAlias.}, see below.

why not use type FooOld {.deprecated.} = FooNew as was done in nim-lang/Nim#13908 ?

for several reasons:

  • this syntax can't be applied to renaming routines due to lack of syntax for aliasing (templates can't do that as explained here [superseded] alias: myecho=echo to alias any symbol Nim#11822)
  • this syntax is not DRY (you're repeating the kind, eg type/var/proc/template etc; the on/off export marker, and the declaration in the case of routines)
  • the deprecation msg is worse (Warning: FooOld2 is deprecated instead of Warning: use Foo instead; FooOld1 is deprecated)
  • it's not a true alias as you can see here:
when true: # D20200428T162935
  type Foo = object
    x: int
  {.deprecated: [FooOld1: Foo].}
  type FooOld2 {.deprecated.} = Foo
  import macros
  {.push experimental:"dynamicBindSym".}
  macro dbg(a): untyped =
    let a = bindSym(a.strVal)
    echo (a.repr, a.typeKind)
  dbg(Foo) # ("Foo", ntyObject)
  dbg(FooOld1) # ("Foo", ntyObject)
  dbg(FooOld2) # ("FooOld2", ntyAlias) # the abstraction leaked as it's not `("Foo", ntyObject)`

{.deprecatedAlias.}

This would be the best approach and we should do it and backport it:

  • it works identically for all declarations (type/var/proc)
  • it solves the partial overload issue elegantly; in particular doesn't need repeating routine declaration
  • it's easier for tooling (eg nimfix was automatically able to upgrade code using {.deprecated: [FooOld: FooNew].} and should be feasible to revive it and teach it {.deprecatedAlias.} )
  • it's more DRY (doesn't need a separate entry)
type Foo = object {.deprecatedAlias: FooOld.}
proc fun(a: int) {.deprecatedAlias: funOld.} # only overload `funOld(int)` is renamed
const bar = 1 {.deprecatedAlias: barOld.}
proc baz(a: int) {.deprecatedAlias: [baz1, baz2].} # multiple successive renames are ok too
{.deprecated: [myfunOld: myfunNew].} # still useful, when we want to rename all overloads

The alternative would be to allowing specifying the signature in the rewrite rule, but it's less appealing:

proc funOld(a: int, b: float): int = discard # this overload shouldn't be renamed
proc fun(a: int): int = discard # this is the new name for `proc funOld(a: int)`
# {.deprecated: [funOld: fun].} # apply to all overloads in scope => can't work here because of funOld(int,float)
{.deprecated: [funOld(int): fun].} # apply only to given signature
{.deprecated: [funOld(aOldParamName: int): fun].} # params can be renamed too

the param names and return type don't need to be specified since you can't overload by these (but you can rename parameters as shown above. The same idea could be applied for selective import, eg: import foo except bar(int, float)

note

code changes, however minor (whether it's a bug fix or new feature or the proposed {.deprecated: [FooOld: FooNew].}) always have the slight potential to break someone's code/workflow, see the classic https://xkcd.com/1172/; because users may implicitly rely on the old behavior (eg here, it could cause conflict if user has introduced a type AssertionDefect in his code); this applies to backports (eg nim-lang/Nim@8cf8879).
However, this is an acceptable tradeoff compared to the alternative, and those breakages should be unlikely and easy to fix.

links

D20200428T154340

summary:

  • introduce {.deprecatedAlias.}
  • backport it to 1.0.X and 1.2.X
  • change all API renames (eg Error -> Defect for defects Nim#13908) to use {.deprecatedAlias.}
  • user code and thirparty packages don't need to do any work and their code keeps working, regardless of 1.0.X, 1.2.X or devel
@timotheecour timotheecour changed the title API renames must be backported API renames must be backported, new {.deprecatedAlias.} syntax Apr 29, 2020
@Araq
Copy link
Member

Araq commented Apr 29, 2020

The problem is real but the solution could also be:

Let's release 1.4 already and accept that the community must move to 1.4. It's semver, we know it doesn't work, yet we stick to it.

@Araq
Copy link
Member

Araq commented Apr 29, 2020

Or: We backport the *Defect names but in the backported version the old names are not deprecated.

@timotheecour
Copy link
Member Author

Let's release 1.4 already and accept that the community must move to 1.4.

That would just kick the can down the road and you'd be faced with the exact same problem the next time around for 1.4=>1.6 or any future transition.
This is not gonna be the last API rename ("famous last words"), and we should acknowledge that and have the means to handle backward compatibility in a sane way with minimal impact for user code.
I don't really care about 1.0 or 1.2 specifically (some do I, maybe), but I care about having a means to handle backward compatibility that doesn't cause extra work for third party code, moving forward.

It's semver, we know it doesn't work, yet we stick to it.

there are 2 options:

  • option 1: semver followed religiously: this guarantees extra work for 3rd party package authors (support old version with different names), until they give up and the whole ecosystem becomes unusable for past releases because of dependencies (including non-nim dependencies (eg homebrew) where fixes would only be for lastest release tags)
  • option 2: semver followed pragmatically: we make the necessary amendments in the tip of each stable branch so that supporting past releases doesn't require so much work for packages

I prefer the pragmatic approach to the "1.0 stability guarantee".

Or: We backport the *Defect names but in the backported version the old names are not deprecated.

That could be a compromise, but I think most ppl stuck on 1.0 (or any past release) want to migrate at some point, and migration is easier if gradual (fighting 1 deprecation warning/legacy flag) at a time.

Suppressing a warning is so trivial though, I don't see how it's a negative: just add --warning:deprecated:off to your ~/.config/nim/config.nims
(but as mentioned in RFC we should introduce --warning:deprecated:bar:off and backport that as it's less blunt)

@zah
Copy link
Member

zah commented Apr 29, 2020

With such a small team, I think Nim still needs to maintain a more rapid development mode if it's going to stay competitive. Putting a lot of effort to support older versions is taxing on this. I think the great majority of active Nim users are always eager to use to latest version with the shiny new features and fixes :)

@arnetheduck
Copy link

API renames

this could be considered a breaking change because it introduces new symbols in the global namespace - due to modules not really being isolated - it's quite common that adding a symbol to one module breaks another (this is a general problem in nim, as soon as you have more than a pageful of code because of how weak the module boundary is)

I think the great majority of active

this is a self-fulfilling prophesy - being "forced" to upgrade regularly prevents any other kind of users to become active nim users - backwards compat is a killer feature that allows a new category of users to participate

practical

compilerProc cannot be changed because new std libraries need to support old compilers - this is why raisesIndexError stayed with that name in the branch - even though in theory a new proc could be added as well - but that would result in two compilerProc names to maintain which is messy

on balance, I'd prefer seeing most renames backported, but it would be nice to do something about the global namespace and module system as well - as-is, it'll be hard to create a library eco-system that doesn't keep breaking

@Araq
Copy link
Member

Araq commented Apr 29, 2020

but it would be nice to do something about the global namespace and module system as well

I have a solution for that but it's "yet another feature" or at least "yet another language change" but maybe I should write the RFC anyway...

@Araq
Copy link
Member

Araq commented Oct 28, 2020

drop support for 1.0 and 1.2 (the most likely outcome, because it's the easiest)

Yeah, too bad. But since API renames always cause friction and in the end the system with the worst names in history wins ("man, cat, grep, cp, kill ...") we simply won't accept API renames anymore in the future.

@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

4 participants