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

[WIP] Proof of concept for versioning the standard library #8309

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

andralex
Copy link
Member

@andralex andralex commented Oct 31, 2021

This PR introduces a very limited v2 of the standard under the name std.v2. There are only very few artifacts in there. Essentially, this PR just showcases how mismatch can be versioned so as to accommodate autodecoding in v1 and no autodecoding in v2.

To that end, v2 supports the following artifacts: std.v2.range.primitives.empty, std.v2.range.primitives.front, std.v2.range.primitives.popFront, std.v2.range.primitives.isInputRange, std.v2.meta.allSatisfy. Some of these are simply pulled from v1, others are redefined with new meaning.

The characteristics of this approach:

  • ZERO duplication of implementation.
  • NO version/static if hell.
  • NO patching or git cherry-picking across versions hell.
  • Total control and flexibility with regard to inheriting symbols across versions with unchanged semantics, or redefining them with changed semantics.
  • Interoperability across versions, including incremental migration.

The main challenges are:

  • Documentation generation must be improved carefully: some documentation must be automatically generated multiple times across versions, some must be overridden, and probably some must be assembled with e.g. v1 + notes specific to v2.
  • Language support for import must be improved. Right now the mixin imports are rather awkward.

@dlang-bot dlang-bot added the WIP Work In Progress - not ready for review or pulling label Oct 31, 2021
@dlang-bot
Copy link
Contributor

dlang-bot commented Oct 31, 2021

Thanks for your pull request, @andralex!

Bugzilla references

Auto-close Bugzilla Severity Description
22596 normal The "publictests" target runs unittests at the top-level namespace so they don't have access to

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8309"

module std.v2.range.primitives;

/**
@@@TODO@@@ Publicly imported names across modules should copy or link those
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@adamdruppe that's close. How easy would it be to modify dpldocs to:

  • actually link to the individual names imported?
  • maybe copy the entire documentation?
  • maybe make the documentation available as a macro for subsequent use?

Copy link
Contributor

Choose a reason for hiding this comment

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

All pretty easy, I just never got around to it (I personally kinda hate selective imports. They're .... really weird when you get into them. I prefer static imports. But since I don't personally use it and there's been no user demand this just never became enough of a priority to actually do. But it'd prolly only take an hour or two.)

import v1 = std.algorithm.comparison;

/**
[@@@TODO@@@ Documentation specific for version 2. It should be appended to the default as
Copy link
Contributor

Choose a reason for hiding this comment

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

This is relatively easy to do in a doc generator, but most aliases probably don't want this behavior. I think if I was going to do it in the adrdox I'd do it with antoher magic macro like

/++
   $(SHOW_ALIAS_DOCS)

   Supplemental stuff here.
+/

And the magic macro there is expanded to be a copy of the thing it is an alias of or something like that.

I'm not sure I love it though, just plain linking is probably a better experience more often than not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yah, I too think a sort of directed choice is best and most flexible.

{
static foreach (r; rs[1 .. $])
{
if (r.empty || !binaryFun!pred(rs[0].front, r.front))
Copy link
Member

Choose a reason for hiding this comment

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

binaryFun with a string predicate imports a bunch of std stuff (including std.range). You need to migrate that as well.

Honestly, ANY functions that you use anywhere should be v2, which should then publicly import the v1 versions if unchanged. Otherwise, any time you want to change a function only for v2, you have to go through every possible usage of it anywhere and upgrade the machinery.

Note that this is kind of implied anyway, because a std.v2 of phobos is going to need to be just as usable as v1 of phobos. But the fact that this worked and pulled in definitions from v1 without explicitly requesting it means this mechanism is prone to subtle v1/v2 mismatch (no pun intended) errors.

I almost think the canon template should be in its own file that isn't accidentally importing v1 stuff.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for taking a look!

binaryFun with a string predicate imports a bunch of std stuff (including std.range). You need to migrate that as well.

Interesting. I'm not so sure. If std.functional.binaryFun version 1 works as needed to implement v2's mismatch then mismatch is free to just use it without migrating it at all. It's a trivial case of code reuse.

Honestly, ANY functions that you use anywhere should be v2, which should then publicly import the v1 versions if unchanged. Otherwise, any time you want to change a function only for v2, you have to go through every possible usage of it anywhere and upgrade the machinery.

I'm unclear on why this is necessary. Clearly in the long run all symbols are migrated, but during the alpha version of v2 implementations therein should be free to use parts of v1 privately wherever appropriate.

Note that this is kind of implied anyway, because a std.v2 of phobos is going to need to be just as usable as v1 of phobos. But the fact that this worked and pulled in definitions from v1 without explicitly requesting it means this mechanism is prone to subtle v1/v2 mismatch (no pun intended) errors.

I think this is a liability only if said v1 artifacts are misused, i.e. not used according to their spec.

I almost think the canon template should be in its own file that isn't accidentally importing v1 stuff.

That would probably be a nice option in the future.

Copy link
Member

@schveiguy schveiguy Nov 2, 2021

Choose a reason for hiding this comment

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

Interesting. I'm not so sure. If std.functional.binaryFun version 1 works as needed to implement v2's mismatch then mismatch is free to just use it without migrating it at all. It's a trivial case of code reuse.

It's not a trivial problem. It will result in subtle WTF moments e.g. an alternate form of your unittest:

auto s1 = ["öabc"], s2 = ["üabc"];
auto a = mismatch!((a, b) => a.front == b.front);
assert(a[0].empty && a[1].empty); // all start with the same byte
auto b = mismatch!"a.front == b.front";
assert(a[0] == s1 && a[1] == s2); // but if using string lambdas, autodecoding is back!

For mismatch, this is probably not going to show up very often, because it has to be a range of ranges AND you must use range functions/algorithms inside your lambda AND it must be a string lambda. But I heavily dislike the subtle inclusion of v1 stuff in corner cases.

Ways to fix:

  1. Fix the string lambda mechanism of binaryFun so it doesn't import ANY niceties if in v2 mode. You want string lambdas? They have to be simple, or use v1 functions.
  2. Remove string lambda support completely from v2. stirng lambdas are kind of obsolete anyway
  3. Fix the code so binaryFun imports v2 versions of all those libraries (and fix all those libraries not to use autodecode). That makes this PR significantly bigger and more complex.

I think this is a liability only if said v1 artifacts are misused, i.e. not used according to their spec.

I think you are headed for a lot of pain if you don't ensure all dependencies go through a v2 gate, even if it's an alias to the v1 version. You did this with allSatisfy and isInputRange, I'm not sure why you would leave out binaryFun.

Copy link
Member

Choose a reason for hiding this comment

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

Further note, tuple and Tuple should be v2 as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Excellent point, thanks. Incidentally I thought (before realizing this issue) that we should repudiate string lambdas as we have reasonably terse and much more principled lambdas now.

It looks like transitive dependencies must be carefully analyzed to figure out what's what.

Any issues you foresee with Tuple and tuple related to unexpected incompatibilities?

Copy link
Member

Choose a reason for hiding this comment

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

Is stdv2.typecons going to exist? If it is, then either it has the same stuff or modified stuff. Having to know this for reviews, usage, etc. is just a pain. I'd rather just everything that is v2-focused import v2 stuff ONLY, unless it is exposing v1 stuff explicitly (i.e. publicly importing symbols, or using the v1 canon template).

If stdv2.typecons is not going to exist, then it's not ambiguous, and possibly OK.

@Imperatorn
Copy link
Contributor

Whatever happens to this, I just want to say: a real attempt is worth 10x more than just discussing forever. It's 's trying to move things forward. Perfection is only found in the platonic universe, once brought out in the physical world, imperfections will always be noticeable.

Brace yourself, v2 is coming

@Imperatorn
Copy link
Contributor

  | std/v2/range/primitives.d(15:24)[warn]: A public function needs to contain a Returns section.
  | std/v2/range/primitives.d(15:24)[warn]: Public declaration 'front' has no documented example.
  | std/v2/range/primitives.d(27:6)[warn]: Parameter a isn't documented in the Params section.
  | std/v2/range/primitives.d(27:6)[warn]: Public declaration 'popFront' has no documented example.
  | std/v2/algorithm/comparison.d(13:1)[warn]: A unittest should be annotated with at least @safe or @System
  | std/algorithm/comparison.d(1851:10)[warn]: Public declaration 'canon' is undocumented.

@adamdruppe
Copy link
Contributor

adamdruppe commented Nov 2, 2021 via email

@@ -0,0 +1,24 @@
module std.v2alpha.algorithm.comparison;
Copy link
Member

Choose a reason for hiding this comment

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

I suggest std2 instead of std.v2. v2 should not be a sub-package of std version 1.

Copy link
Contributor

@Imperatorn Imperatorn Nov 2, 2021

Choose a reason for hiding this comment

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

Kinda makes sense. It would be a different story if we had this in the beginning, like if it already was std.v1 etc and now std.v2 etc.

General question arises (excuse me if this is already answered in the forums):
Would these be side by side?

Will you be able to selectively import stuff from std while still using std2?

import std;
import std2 : awesome;

import std2;
import std : awesome;

No conflict because the selective import always wins?


/**
On an individual public import, it would be very helpful if the documentation
for that symbol were copied here (or available as a macro).
Copy link
Member

Choose a reason for hiding this comment

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

For now it can be simply linked to with the $(LINK ...) macro.

/**
@@@TODO@@@ This function redefines `front` for v2, meaning its documentation
will override the documentation of `front` found in v1. The difference is of
course that the new `front` does not autodecode.
Copy link
Member

Choose a reason for hiding this comment

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

Adding a link to the v1 documentation makes it easier for people to see what's been updated.

Copy link
Member

@WalterBright WalterBright left a comment

Choose a reason for hiding this comment

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

Very nice! Just change it to std2 and it's good to go.

@Imperatorn
Copy link
Contributor

stdv2 should be on the same level as std so the directory structure will be the same

{
assert(a.length, "Attempting to popFront() past the end of an array of " ~ T.stringof);
a = a[1 .. $];
}
Copy link
Member

@burner burner Nov 2, 2021

Choose a reason for hiding this comment

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

what about

private deprecated void popFrontString(scope ref string a) {
    // auto decode impl
}

void popFront(T)(scope ref inout(T)[] a) {
    static assert(!is(T[] == void[]), T.stringof ~ "[] not a support range");
    static if(is(T == string)) {
        popFrontString(a);
    } else {
        a = a[1 .. $];
    }
}

No need for std2, you should get a deprecation message ... move on

Copy link
Member

Choose a reason for hiding this comment

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

No, we don't want to remove strings as ranges.

Copy link
Member

Choose a reason for hiding this comment

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

after the deprecation phase we would remove everything but the first static assert and the a = a[1 .. $];

strings should still work, only without autodecoding.

What did I miss?

@andralex
Copy link
Member Author

andralex commented Nov 6, 2021

Another bug in the compiler revealed by unittests:

../dmd/generated/linux/release/64/dmd  -conf= -I../druntime/import  -w -de -preview=dip1000 -preview=dtorfields -preview=fieldwise -m64 -fPIC -O -release -defaultlib= -debuglib= -L-lpthread -L-lm -betterC -unittest -run generated/linux/release/64/betterctests/std_algorithm_comparison.d

issues the nonsensical error:

std/array.d(3312): Error: `TypeInfo` cannot be used with -betterC

The line is bogus and the error is bogus.

@andralex
Copy link
Member Author

FYI the current implementation breaks the navigation in the documentation, see e.g. std.algorithm and std.algorithm.searching

Affirmative. It's a bug I'll submit in a few hours that needs to be fixed before this PR can be merged.

@MoonlightSentinel
Copy link
Contributor

Good. But it's a red flag that an implementation detail like canon leaks into the documentation at all.

@dukc
Copy link
Contributor

dukc commented Dec 16, 2021

I think you should stop adding new functions and get what you have already made merged first. That way others can also begin porting the functions.

@andralex
Copy link
Member Author

I think you should stop adding new functions and get what you have already made merged first. That way others can also begin porting the functions.

I wish things worked out that way, and that was my hope starting this. However, there are a couple of problems that make that impossible. Loosely in decreasing order of importance:

  1. The restructuring of the code makes the generated documentation unusable.
  2. In order to figure out what documentation generation features to ask for, we need to figure what code structure scales best
  3. In order to figure out scaling, we need to experiment at some scale so as to assess the issues when multiple interdependent modules are being versioned.
  4. In the process of doing all that, there are bugs in the compiler that need fixing before merging.

So... not a simple or quick process.

@dukc
Copy link
Contributor

dukc commented Dec 17, 2021

1. The restructuring of the code makes the generated documentation unusable.

2. In order to figure out what documentation generation features to ask for, we need to figure what code structure scales best

3. In order to figure out scaling, we need to experiment at some scale so as to assess the issues when multiple interdependent modules are being versioned.

4. In the process of doing all that, there are bugs in the compiler that need fixing before merging.

Just disable the docs for now. It's going to require future PRs anyway before v2 is ready for any serious use, and we're definitely not going to worry about breaking the code of any experimenters. So no need to do it all in one monolith PR.

@andralex
Copy link
Member Author

Just disable the docs for now.

How do you mean that?

@dukc
Copy link
Contributor

dukc commented Jan 25, 2022

Anyone who knows a bit about docs, what would be the best way to hide the documentation of V2 for now without affecting V1?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Rebase Needs Work stalled WIP Work In Progress - not ready for review or pulling
Projects
None yet
10 participants