Add enum discriminants #337

SuperSonicHub1 · 2024-07-21T22:05:28Z

Adds information about enum discriminants to the schema.

Refs:

SuperSonicHub1 · 2024-07-21T22:12:17Z

Also, on why repr is not discussed in this PR:

the Discriminant rustdoc type contains no info about repr :/
repr is an attr, so that should probably be the responsibility of Enum, not Discriminant
parsing the repr attr is complex, so it likely should be a separate issue

obi1kenobi

I don't think we can get away from parsing the enum's repr, since the existence of an explicit repr sometimes causes discriminants to be defined implicitly. I suspect those implicit discriminants might not be part of rustdoc JSON, but I've suggested a test that can check that.

There's already some attribute-parsing code in the adapter, so you don't have to reinvent the wheel there.

src/adapter/vertex.rs

src/rustdoc_schema.graphql

src/adapter/tests.rs

test_crates/enum_discriminants/src/lib.rs

SuperSonicHub1 · 2024-07-22T01:01:53Z

To more generally address some of the previous comments due to me leaving stuff out of docs:

rustdoc only lists explicit discriminations, so we'll have to do the calculations for implicit discriminations ourselves. I'll add this in a new patch; let me know if we should think on this instead. (I assume we're going to squash this PR. :))
value is always a number, but it's in the range [-(2^127), 2^128) according to the docs, so I would agree with the rustdoc devs that it should stay a string. If this would be a limitation on cargo-semver-checks, we'll need to either add bigints or an i256 software impl to Trustfall.

The numerical value of the discriminant. Stored as a string due to JSON’s poor support for large integers, and the fact that it would need to store from i128::MIN to u128::MAX.

SuperSonicHub1 · 2024-07-22T01:12:14Z

rustdoc only lists explicit discriminations, so we'll have to do the calculations for implicit discriminations ourselves.

I just remembered that the datatype of value is complicated due to repr, which I currently don't support. :)

I'll hold off on that until I hear back from you.

obi1kenobi

I like where this is going! Thanks for putting it together!

I think the major design question is what to do with implicitly-defined discriminants.

Since lints care about whether the discriminant's value is well-defined, and don't care about how it's defined, I think it'll be more helpful for lints if we always provide the discriminant if it's well-known.

I made a case for this within.

src/rustdoc_schema.graphql

obi1kenobi · 2024-07-22T03:49:22Z

src/rustdoc_schema.graphql

+  expr: String!
+
+  """
+  The numerical value of the discriminant. Stored as a string. Ranges from `i128::MIN` to `u128::MAX`.


Minor phrasing tweak suggestion:

Suggested change

The numerical value of the discriminant. Stored as a string. Ranges from `i128::MIN` to `u128::MAX`.

The numerical value of the discriminant, represented as a string.

May range from `i128::MIN` to `u128::MAX`.

test_crates/enum_discriminants/src/lib.rs

obi1kenobi · 2024-07-22T04:04:10Z

I just remembered that the datatype of value is complicated due to repr, which I currently don't support. :)

I think it's probably fine to keep it as a string.

If you don't want to include the data type in the schema, you don't have to. I don't think it'll be strictly necessary to compute discriminant values, since they'll get turned into strings anyway.

It might be worth trying to add a fast path that avoids 128-bit integer math, and uses 64-bit math as much as possible when computing implicit discriminants. 128-bit ints can be quite expensive, and almost no enums will actually use them in practice.

Co-authored-by: Predrag Gruevski <[email protected]>

SuperSonicHub1 · 2024-07-22T15:34:02Z

If you don't want to include the data type in the schema, you don't have to. I don't think it'll be strictly necessary to compute discriminant values, since they'll get turned into strings anyway.

I think repr(..) is a complex topic; I want to avoid overscoping this PR.

It might be worth trying to add a fast path that avoids 128-bit integer math, and uses 64-bit math as much as possible when computing implicit discriminants. 128-bit ints can be quite expensive, and almost no enums will actually use them in practice.

Even if I had the type, parsing explicit discriminants is hard because I don't know the type until I a number becomes too large or too small for the type I presumed. Below is what I believe is the minimum code for representing any possible discriminant value. I believe use of unwrap is justified as rustc has already made sure that all enum discriminants fit inside either an u128 or an i128. Let me know if it looks good.

enum DiscriminantValue {
    I64(i64),
    U64(u64),
    I128(i128),
    U128(u128),
}

impl DiscriminantValue {
    fn increment(self) -> DiscriminantValue {
        match self {
            DiscriminantValue::I64(i) => {
                match i.checked_add_unsigned(1) {
                    Some(i) => i.into(),
                    None => {
                        match u64::try_from(i) {
                            Ok(u) => DiscriminantValue::from(u).increment(),
                            // i is a negative i128
                            Err(_) => DiscriminantValue::from(i128::try_from(i).unwrap()).increment(),
                        }
                    },
                }
            },
            DiscriminantValue::U64(i) => {
                match i.checked_add(1) {
                    Some(i) => i.into(),
                    None => {
                        match i128::try_from(i) {
                            Ok(u) => DiscriminantValue::from(u).increment(),
                            // i is a u128
                            Err(_) => DiscriminantValue::from(u128::try_from(i).unwrap()).increment(),
                        }
                    },
                }
            },
            DiscriminantValue::I128(i) => {
                match i.checked_add_unsigned(1) {
                    Some(i) => i.into(),
                    // i is a u128
                    None => DiscriminantValue::from(u128::try_from(i).unwrap()).increment(),
                }
            },
            DiscriminantValue::U128(i) => (i + 1).into(),
        }
    }
}

impl From<i64> for DiscriminantValue {
    fn from(value: i64) -> Self {
        DiscriminantValue::I64(value)
    }
}

impl From<i128> for DiscriminantValue {
    fn from(value: i128) -> Self {
        DiscriminantValue::I128(value)
    }
}

impl From<u64> for DiscriminantValue {
    fn from(value: u64) -> Self {
        DiscriminantValue::U64(value)
    }
}

impl From<u128> for DiscriminantValue {
    fn from(value: u128) -> Self {
        DiscriminantValue::U128(value)
    }
}

impl FromStr for DiscriminantValue {
    type Err = ParseIntError;

    
    fn from_str(s: &str) -> Result<Self, Self::Err> {
        if let Ok(i) = i64::from_str(s) {
            return Ok(i.into());
        }
        if let Ok(i) = u64::from_str(s) {
            return Ok(i.into());
        }
        if let Ok(i) = i128::from_str(s) {
            return Ok(i.into());
        }
        match u128::from_str(s) {
            Ok(i) => Ok(i.into()),
            Err(e) => Err(e),
        }
    }
}

obi1kenobi · 2024-07-22T21:19:01Z

I have a few concerns that could be resolved by seeing more of how you plan to integrate that enum into the broader design:

I'm wary of including lots of enum nesting each with only a few variants at a time. Each time we nest enums, we pay for a fresh discriminant which costs memory, which in turn costs performance.
Relatedly, that enum is quite large (at least 17 bytes) and in the vast majority of cases will contain 16 bytes of zeroes. That will also have a performance cost, especially if we include it in the vertex.
I think at least one branch in the shown increment() function is actually unreachable: an i64 being incremented by 1 must always fit either in i64 or u64. It cannot require i128 because it cannot become any more negative, nor exceed u64::MAX while starting from i64::MAX. More careful analysis would be useful to minimize the amount of code there.

SuperSonicHub1 · 2024-07-25T20:29:41Z

Spent some time while on the move simplifying increment and sketching out discriminant assignment: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f61c7eb6a08c32afee8d13a88a36f6f6

Test looks like it demonstrates expected behavior; perhaps should include more pathological results like i128 and u64 instances. If you think of a more functional and less mutable way of calculating the discriminants before I do, let me know. I'll more neatly integrate the algo into the codebase in terms of lifetimes once I'm at my desk again.

I think I'm going to calculate the discriminants all at once when the variants edge is requested. The algorithm is linear, so it shouldn't take very long for non-pathological codebases. Passing around a &'a String with a variant's Id shouldn't be too bad, I think.

Finally, I have decided that I'm going to remove Discriminant::expr from the schema; it's really only present for cosmetic reasons relevant to Rustdoc and not us, and has no use in any of the lints you have planned. As a wise mentor (you) once told me long ago, "only implement what you need."

obi1kenobi · 2024-07-25T20:38:45Z

This looks quite reasonable, thank you!

I'd recommend a couple of small-ish tweaks, in addition to recommending more test coverage with i128 and u128 discriminants before we merge.

Consider either increment(&self) or increment(&mut self) instead of increment(self)? The underlying type is Copy so the move doesn't look to me like it's buying us very much.

Also, consider (either in this PR or in a subsequent one) making it so that the discriminants are only computed when a discriminant edge is requested, not a variant edge? This is because there will be lots of lints that touch variants without touching discriminants, and we'd prefer to be as lazy as possible here. Ideally, we'd also memoize the computed discriminant strings if possible so we don't compute them multiple times if we have multiple lints over them.

The outcome we'd like to avoid in the long run is having many lints repeatedly compute discriminant strings (and do a bunch of allocations in the process) even though very few of them actually ever need discriminants.

SuperSonicHub1 · 2024-07-25T21:26:45Z

Also, consider (either in this PR or in a subsequent one) making it so that the discriminants are only computed when a discriminant edge is requested, not a variant edge? This is because there will be lots of lints that touch variants without touching discriminants, and we'd prefer to be as lazy as possible here.

I was thinking about how to do the former the other day; the Rustdoc just makes it really annoying to get a Variant's parent Enum and so its sibling enums. This is necessary as a variant's discriminant conditionally depends on the variants before it.

Ideally, we'd also memoize the computed discriminant strings if possible so we don't compute them multiple times if we have multiple lints over them.

Does Trustfall have some kind of general caching utility? If not, caching for our use case could be easily achieved with once_cell.

obi1kenobi · 2024-07-25T21:55:46Z

I was thinking about how to do the former the other day; the Rustdoc just makes it really annoying to get a Variant's parent Enum and so its sibling enums. This is necessary as a variant's discriminant conditionally depends on the variants before it.

Does Trustfall have some kind of general caching utility? If not, caching for our use case could be easily achieved with once_cell.

Here's what I'd do.

Right now, variants are represented as VertexKind::Item. I'd consider carving out a separate variant, VertexKind::Variant(EnumVariant<'a>) for them, where EnumVariant looks roughly like this:

#[non_exhaustive]
#[derive(Debug, Clone)]
pub struct EnumVariant<'a> {
    item: &'a Item,
    discriminant: todo!("< something clever that holds the parent `&'a Enum` internally and is lazily initialized >")
}

Then on first access to the discriminant field, we can compute all the discriminants for all the variants of that enum. This would imply some Rc inside there + a variant-specific offset or another way to indicate which of the bulk-computed discriminants corresponds to the current variant.

This sidesteps the problem of getting the &'a Enum for a variant, since we construct the EnumVariant<'a> value while we still know the &'a Item + &'a Enum that own the variant — we're coming from that edge.

SuperSonicHub1 · 2024-07-26T00:12:18Z

Realized that in order to do the first thing, I'll be 90% of the way to doing the other, so might as well do it all in one patchset.

SuperSonicHub1 · 2024-07-26T20:26:54Z

After much fighting with rustc and two afternoons of rumination, I got some working tests. Please suggest ways for me to clone less and be more memory efficient.

Will add OrderedVariant (do you think simply adding index to the Variant type would be better? I do) after a much deserved break.

SuperSonicHub1 · 2024-07-26T20:37:01Z

Messed up CI; on that.

SuperSonicHub1 · 2024-07-26T20:42:08Z

Aside: getting pre-commit hooks going for cargo clippy && cargo fmt && cargo doc && cargo test would be appreciated.

obi1kenobi

This is looking pretty good!

I made some suggestions on how to avoid some cloning and eager evaluations to further improve performance. Let me know if you'd like to pair-program any of this — sometimes working with iterators and lifetimes gets complicated, and I've debugged a lot of those errors so I'm happy to help.

Re: pre-commit hooks, I actually really hate tools that prevent me from committing since I do a lot of quick-and-dirty experimentation. But if you wanted to set it up for yourself in a way that doesn't impose a mandatory workflow change on other contributors, I'm fine with that.

src/adapter/enum_variant.rs

src/adapter/edges.rs

src/adapter/enum_variant.rs

src/adapter/vertex.rs

SuperSonicHub1 · 2024-08-31T18:41:26Z

We'll come back to ordered variants another time.

SuperSonicHub1 · 2024-08-31T18:41:46Z

Otherwise, the patch should be done now!

obi1kenobi

Nice! Just some test improvements and polish items left. There appears to be a merge conflict too, which should be an easy fix.

I've retargeted the PR to branch rustdoc-v33 instead of rustdoc-v28, so that we merge into the current development branch. That way I can easily port this to older versions, since I have some automation to handle that.

src/adapter/tests.rs

src/rustdoc_schema.graphql

test_crates/enum_discriminants/src/lib.rs

src/rustdoc_schema.graphql

Co-authored-by: Predrag Gruevski <[email protected]>

SuperSonicHub1 · 2024-09-01T16:00:34Z

I believe this should be it.

SuperSonicHub1 · 2024-09-01T16:00:57Z

Also, CI's been acting a bit weird these past few days; would you know why?

obi1kenobi · 2024-09-01T16:09:19Z

Still seeing merge conflicts on my end, can you try pulling the latest upstream rustdoc-v33 branch and merging it into your branch?

GitHub doesn't trigger CI runs on PRs that are unmergeable due to conflicts.

SuperSonicHub1 · 2024-09-01T16:19:20Z

GitHub doesn't trigger CI runs on PRs that are unmergeable due to conflicts.

D'oh. Think I did it.

.github/workflows/ci.yml

obi1kenobi

Excellent, thank you!

Any interest in continuing to work on variant order / struct field order? Or too busy with schoolwork? No pressure — cargo-semver-checks will be here if/when you want to come back to contributing — I just wanted to know whether you wanted dibs on those extensions or if I should put them up for grabs.

* Remove nightly from rustdoc v28 test matrix, since that's now v29. (#332) * Rust 1.78 is no longer beta. (#334) * Add enum discriminants * discriminant clone to lifetime annotation * complicate test examples * improved discriminant docs * hedge bets against placeholder representation Co-authored-by: Predrag Gruevski <[email protected]> * finally got implicit discriminants working * remove sync structs * move to Cow<'a, str> for discriminants * Various clean-ups, still fighting closure bounds *sigh* error[E0521]: borrowed data escapes outside of closure --> src/adapter/edges.rs:304:28 | 260 | pub(super) fn resolve_variant_edge<'a, V: AsVertex<Vertex<'a>> + 'a>( | -- lifetime `'a` defined here ... 302 | "discriminant" => resolve_neighbors_with(contexts, move |vertex: &'_ Vertex<'a>| { | ------ - let's call the lifetime of this reference `'1` | | | `vertex` is a reference that is only valid in the closure body 303 | let origin = vertex.origin; 304 | let enum_var = vertex | ____________________________^ 305 | | .as_variant() | | ^ | | | | |_____________________________`vertex` escapes the closure body here | argument requires that `'1` must outlive `'a` * Actually clone Cow; same error as before, though... * Got the sucker. * Wrap up * Better dcocs for discriminant Co-authored-by: Predrag Gruevski <[email protected]> * Finish up docs, improve tests * Add name back. --------- Co-authored-by: Predrag Gruevski <[email protected]>

* Remove nightly from rustdoc v28 test matrix, since that's now v29. (#332) * Rust 1.78 is no longer beta. (#334) * Add enum discriminants * discriminant clone to lifetime annotation * complicate test examples * improved discriminant docs * hedge bets against placeholder representation * finally got implicit discriminants working * remove sync structs * move to Cow<'a, str> for discriminants * Various clean-ups, still fighting closure bounds *sigh* error[E0521]: borrowed data escapes outside of closure --> src/adapter/edges.rs:304:28 | 260 | pub(super) fn resolve_variant_edge<'a, V: AsVertex<Vertex<'a>> + 'a>( | -- lifetime `'a` defined here ... 302 | "discriminant" => resolve_neighbors_with(contexts, move |vertex: &'_ Vertex<'a>| { | ------ - let's call the lifetime of this reference `'1` | | | `vertex` is a reference that is only valid in the closure body 303 | let origin = vertex.origin; 304 | let enum_var = vertex | ____________________________^ 305 | | .as_variant() | | ^ | | | | |_____________________________`vertex` escapes the closure body here | argument requires that `'1` must outlive `'a` * Actually clone Cow; same error as before, though... * Got the sucker. * Wrap up * Better dcocs for discriminant * Finish up docs, improve tests * Add name back. --------- Co-authored-by: Kyle Anthony Williams <[email protected]>

SuperSonicHub1 · 2024-09-01T17:21:29Z

Woo!

SuperSonicHub1 · 2024-09-01T17:24:09Z

Any interest in continuing to work on variant order / struct field order?

I would, but school and research is likely going to be tough, so put them up for grabs. Plan for this summer is to find a way to get paid to contribute to OSS, so this will not be the last time you hear from me for the next 12 months. :) I also will have a ton of free time during winter break, so we should discuss possibilities then, too.

obi1kenobi · 2024-09-01T17:37:40Z

If there are companies or programs you're interested in applying to, I'd be happy to serve as a reference / write you a recommendation letter / refer you if I know someone there. (Same goes for anyone who consistently contributes quality code to projects I maintain!)

I'm a mentor for Google Summer of Code, and from my perspective that program has been good! Not sure how companies / hiring managers feel about it on a resume (as opposed to say a tech company internship) so it's worth looking into that as well.

Also happy to work together over winter break — ping me whenever it's a good time to chat about that!

obi1kenobi added 2 commits April 17, 2024 02:41

Remove nightly from rustdoc v28 test matrix, since that's now v29. (o…

035e4aa

…bi1kenobi#332)

Rust 1.78 is no longer beta. (obi1kenobi#334)

a0a13fb

Add enum discriminants

4bd03a1

SuperSonicHub1 force-pushed the discriminants branch from 6cde9fe to 4bd03a1 Compare July 22, 2024 00:32

obi1kenobi reviewed Jul 22, 2024

View reviewed changes

SuperSonicHub1 added 3 commits July 21, 2024 21:18

discriminant clone to lifetime annotation

a9bbecb

complicate test examples

7a8ea8a

improved discriminant docs

693ffce

obi1kenobi reviewed Jul 22, 2024

View reviewed changes

hedge bets against placeholder representation

7e30463

Co-authored-by: Predrag Gruevski <[email protected]>

SuperSonicHub1 changed the title ~~Add enum discriminants~~ Add enum discriminants and ordered variants Jul 26, 2024

SuperSonicHub1 force-pushed the discriminants branch 2 times, most recently from 82dfa3e to 3f99cd7 Compare July 26, 2024 20:39

finally got implicit discriminants working

8f50c70

SuperSonicHub1 force-pushed the discriminants branch from 3f99cd7 to 8f50c70 Compare July 26, 2024 20:41

obi1kenobi reviewed Jul 27, 2024

View reviewed changes

SuperSonicHub1 changed the title ~~Add enum discriminants and ordered variants~~ Add enum discriminants Aug 31, 2024

Wrap up

836b5f5

obi1kenobi changed the base branch from rustdoc-v28 to rustdoc-v33 September 1, 2024 14:41

obi1kenobi reviewed Sep 1, 2024

View reviewed changes

src/adapter/tests.rs Outdated Show resolved Hide resolved

src/rustdoc_schema.graphql Outdated Show resolved Hide resolved

test_crates/enum_discriminants/src/lib.rs Show resolved Hide resolved

src/rustdoc_schema.graphql Show resolved Hide resolved

Better dcocs for discriminant

9ace865

Co-authored-by: Predrag Gruevski <[email protected]>

Finish up docs, improve tests

c164df6

SuperSonicHub1 force-pushed the discriminants branch from b77a518 to c164df6 Compare September 1, 2024 16:16

Merge remote-tracking branch 'remote/rustdoc-v33' into discriminants

51349b1

obi1kenobi reviewed Sep 1, 2024

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

Add name back.

e7aa5f3

obi1kenobi approved these changes Sep 1, 2024

View reviewed changes

obi1kenobi merged commit 052469d into obi1kenobi:rustdoc-v33 Sep 1, 2024
6 checks passed

obi1kenobi mentioned this pull request Sep 1, 2024

New lints: Changes to enum variant discriminants obi1kenobi/cargo-semver-checks#898

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add enum discriminants #337

Add enum discriminants #337

SuperSonicHub1 commented Jul 21, 2024

SuperSonicHub1 commented Jul 21, 2024

obi1kenobi left a comment

SuperSonicHub1 commented Jul 22, 2024

SuperSonicHub1 commented Jul 22, 2024

obi1kenobi left a comment

obi1kenobi Jul 22, 2024

obi1kenobi commented Jul 22, 2024

SuperSonicHub1 commented Jul 22, 2024 •

edited

Loading

obi1kenobi commented Jul 22, 2024

SuperSonicHub1 commented Jul 25, 2024

obi1kenobi commented Jul 25, 2024

SuperSonicHub1 commented Jul 25, 2024

obi1kenobi commented Jul 25, 2024 •

edited

Loading

SuperSonicHub1 commented Jul 26, 2024 •

edited

Loading

SuperSonicHub1 commented Jul 26, 2024 •

edited

Loading

SuperSonicHub1 commented Jul 26, 2024 •

edited

Loading

SuperSonicHub1 commented Jul 26, 2024

obi1kenobi left a comment

SuperSonicHub1 commented Aug 31, 2024

SuperSonicHub1 commented Aug 31, 2024

obi1kenobi left a comment

SuperSonicHub1 commented Sep 1, 2024

SuperSonicHub1 commented Sep 1, 2024

obi1kenobi commented Sep 1, 2024

SuperSonicHub1 commented Sep 1, 2024 •

edited

Loading

obi1kenobi left a comment

SuperSonicHub1 commented Sep 1, 2024

SuperSonicHub1 commented Sep 1, 2024

obi1kenobi commented Sep 1, 2024

-  The numerical value of the discriminant. Stored as a string. Ranges from `i128::MIN` to `u128::MAX`.
+  The numerical value of the discriminant, represented as a string.
+  May range from `i128::MIN` to `u128::MAX`.

Add enum discriminants #337

Add enum discriminants #337

Conversation

SuperSonicHub1 commented Jul 21, 2024

SuperSonicHub1 commented Jul 21, 2024

obi1kenobi left a comment

Choose a reason for hiding this comment

SuperSonicHub1 commented Jul 22, 2024

SuperSonicHub1 commented Jul 22, 2024

obi1kenobi left a comment

Choose a reason for hiding this comment

obi1kenobi Jul 22, 2024

Choose a reason for hiding this comment

obi1kenobi commented Jul 22, 2024

SuperSonicHub1 commented Jul 22, 2024 • edited Loading

obi1kenobi commented Jul 22, 2024

SuperSonicHub1 commented Jul 25, 2024

obi1kenobi commented Jul 25, 2024

SuperSonicHub1 commented Jul 25, 2024

obi1kenobi commented Jul 25, 2024 • edited Loading

SuperSonicHub1 commented Jul 26, 2024 • edited Loading

SuperSonicHub1 commented Jul 26, 2024 • edited Loading

SuperSonicHub1 commented Jul 26, 2024 • edited Loading

SuperSonicHub1 commented Jul 26, 2024

obi1kenobi left a comment

Choose a reason for hiding this comment

SuperSonicHub1 commented Aug 31, 2024

SuperSonicHub1 commented Aug 31, 2024

obi1kenobi left a comment

Choose a reason for hiding this comment

SuperSonicHub1 commented Sep 1, 2024

SuperSonicHub1 commented Sep 1, 2024

obi1kenobi commented Sep 1, 2024

SuperSonicHub1 commented Sep 1, 2024 • edited Loading

obi1kenobi left a comment

Choose a reason for hiding this comment

SuperSonicHub1 commented Sep 1, 2024

SuperSonicHub1 commented Sep 1, 2024

obi1kenobi commented Sep 1, 2024

SuperSonicHub1 commented Jul 22, 2024 •

edited

Loading

obi1kenobi commented Jul 25, 2024 •

edited

Loading

SuperSonicHub1 commented Jul 26, 2024 •

edited

Loading

SuperSonicHub1 commented Jul 26, 2024 •

edited

Loading

SuperSonicHub1 commented Jul 26, 2024 •

edited

Loading

SuperSonicHub1 commented Sep 1, 2024 •

edited

Loading