Automatic Generic-Derived Qualifiers #145

Dessix · 2021-08-28T08:51:52Z

Fixes #79 compliant with feedback from #143.

This PR attempts to produce bounds for generics only where necessitated by their usages.

Dessix · 2021-08-28T08:57:54Z

@dtolnay I'm looking for two pieces of advice on this PR:

How to best go about parsing format-literals, hopefully without adding significant dependencies (See attr.rs:37)
A better way to determine when a Type is- or worse- uses one of the generics from the context (See expand.rs:311, especially with the troublesome, verbose pattern on TypeReference)

With the latter, I'll be able to finish implementation of the generics handling for #[from] implementations, and will subsequently add support for structs. From there, it's a matter of adding UI tests

dtolnay

the format parser is in impl/src/fmt.rs.
I think traversing type parameters of fields of type Type::Path should be sufficient. I don't think the complexity of making this more exhaustive is warranted.

Dessix · 2021-08-28T23:57:14Z

@dtolnay: Thanks on the fmt reference- I must have missed that. It looks like I'll need to restructure it to get at the information I'm looking for, however.

As for not going deeper than first-layer Path types- that doesn't cover #[from] cases where we need to determine if a generic is indirectly used in the parameters of another type, unless I'm mistaken?

enum FromGenericError<Indirect> {
    #[error("Tadah")]
    SourceEmbedded(#[from] DirectEmbedding<Indirect>),
}

Isolating the #[from] usage of Indirect appears to be necessary to produce:

impl<Indirect> Error for FromGenericError<Indirect>
where
    DirectEmbedding<Indirect>: Error,
    Indirect: 'static, // <--- This line
    Self: Debug + Display;

As you can see from my latest push, I am looking at using syn::visit to cover deep scanning of the contents for any TypePath with that identifier. Do you have any objection to adding the syn/visit feature as a requirement, or to handling this scenario this way?

…constraint-detection

Dessix · 2021-08-30T10:40:50Z

@dtolnay I attempted to use syn's Parser trait to work with it, but the problem was that format templates are not valid Rust at the tokenizer level. In the end, I did a hybrid combination of your while let loop and a string-content refinement method. It's really rough, and I'd appreciate any rewrites or suggestions.

The tests cover the cases you described in this comment, and they are all now passing.

Dessix · 2021-09-04T01:04:02Z

I realized that this implementation does not yet support #[transparent]. I'm adding some tests to verify it, and attempting to add support.

With the amount of additional code necessary to add it, it may be worth doing as a separate PR. Similarly, we may want to set up tasks for generic support in #[backtrace] and #[source] usages.

One thing I've noticed, @dtolnay, is significant redundancy between both the struct and enum implementations, and between the sub-attribute implementations for each one.

Do you have any ideas on how we can refactor expand.rs to reduce this redundancy? Perhaps we could produce a common intermediate structure which we can reflect against when generating code?

Dessix · 2021-09-04T02:46:37Z

I just spent a few hours attempting to make #[error(transparent)] work. Due to significant coupling between #[source] and #[backtrace] behaviour, I am having a very difficult time implementing transparent over generics.

As I suspected in my prior comment, I think support for these other modes (transparent included) should be done as separate PRs, as this generic functionality is still quite helpful as it stands. Additionally, stopping here allows us to create a PR to refactor the expander into a "usages"-oriented intermediate structure as suggested above.

…derivation Replaced `#[error(transparent)]` usages, as they are not yet supported in automatic generic constraint derivation. Removed generic bounds from `TunnelRegistrationError` and `TunnelNamingError` as they are conventionally incorrect and made unnecessary with the new bound-generation capability. See `thiserror` PR dtolnay/thiserror#145

dtolnay

I ended up implementing this in an easier and less brittle way in #148, so I'll go ahead and close this implementation. Thanks anyway for the PR!

dtolnay · 2021-09-05T03:15:43Z

impl/src/fmt.rs

@@ -94,6 +101,122 @@ impl Display<'_> {
        self.args = args;
        self.has_bonus_display = has_bonus_display;
    }
+
+    pub fn iter_fmt_types(&'a self, fields: &'a [Field]) -> Vec<DisplayFormatMarking<'a>> {


It's problematic that this 172 lines of changes in fmt.rs is duplicating the entire format string parser in a way that is completely detached from (and incompatible with) the existing format string processing.

Take a look at my version which sticks a comparatively tiny piece of code (24 lines) in the existing format string codepath to wire up implied bounds as a natural part of processing the format string.

Yeah, using a member on the Display type was a better move. I like that it also allows bounds for the other display types. I think the one thing I'd change given infinite time would be splitting Display<'_> into pre-expanded and post-expanded types, and keeping the template and non-template sections around as an ADT, then resolving the field rename hacks on demand.

dtolnay · 2021-09-05T03:17:36Z

impl/src/fmt.rs

+    };
+
+    // Strip expanded identifier-prefixes since we just want the non-shorthand version
+    let name = if name.starts_with("field_") {


This is a red flag – this code is trying to reverse engineer (lossily) information that already exists where the format string was already processed inside expand_shorthand.

Agreed- that's a consequence of invoking it at the late-phase location I did- I think I probably should've augmented it by changing expand_shorthand to some sort of generic type that maps it out, but the problem was that my generic intersection code required knowledge of the generics before it could determine what was a generic.

The majority of my time ended up sunk into writing parse methods using syn::Parser, only to find out that it couldn't handle imbalanced bracketing at any point, and didn't provide lower-level access than tokens. Having not spent much time with iterative parsers like the one in expand_shorthand, I couldn't see a straightforward way to extract the full information I needed from it without re-plumbing its state outputs.

Alas- as you showed- that was the right move ^^;

dtolnay · 2021-09-05T03:19:01Z

impl/src/expand.rs

+            .display
+            .iter()
+            .flat_map(|d| d.iter_fmt_types(input.fields.as_slice()))
+        {


In general these changes in expand.rs are way more code than I would expect to need to add for this feature. Please see #148 for the approach I used instead.

Your Field.contains_generic approach combined with the scope abstraction seem to be the biggest contributors to code reduction, and InferredBounds is doing some serious work considering its length.

I'm curious, however, if there's some way to describe the "intent" for each bound as it's read, allowing a single InferredBounds-esque type to track the various usage forms to generalize the solution beyond error and display bounds.

Yes that is how https://github.com/dtolnay/reflect works.

I'll keep an eye out for opportunities to use that library- but I suspect it would be a bit much in the way of overhead for this task? Then again, being a proc macro, its dependencies (hopefully?) aren't compiled into the binary it helps to produce unless explicitly requested.

dtolnay · 2021-09-05T03:20:23Z

impl/src/expand.rs

+        self.generics.iter().filter(|(_k, v)| **v).map(|(k, _v)| k)
+    }
+
+    #[allow(dead_code)]


No dead code please.

My bad- that one got left behind- I must've forgotten to tag it todo and wasn't clear I'd use it or not before the feature was ready.

dtolnay · 2021-09-05T03:23:22Z

impl/src/expand.rs

+        syn::visit::visit_type_path(self, i);
+    }
+
+    fn visit_type_param(&mut self, i: &'ast syn::TypeParam) {


This is not reachable — except maybe in contrived code like field: [T; { fn f<T>() {}; 0 }] where you actually don't want the behavior here of considering the inner T TypeParam as being a use of the outer T.

I used this to deep-search for generic usages because- when a generic wasn't used- I was running into circumstances where fixed types would succeed in compilation where they should not, because it generated impossible/unlikely bounds, such as PathBuf: Display, which you seem to have resolved using the PathAsDisplay trait in the past. My solution was to deep-scan for generics in types, and only add bounds for fields which used at least one generic.

dtolnay · 2021-09-05T03:25:24Z

impl/src/fmt.rs

+
+        member_refs
+            .iter()
+            .map(|(_m, f, is_display)| {


_m shouldn't be getting put into member_refs if nothing is going to use it.

dtolnay · 2021-09-05T03:28:30Z

impl/src/expand.rs

+    pub fn mark_from<'a, TMarkedSource: IntoIterator<Item = &'a proc_macro2::Ident> + 'a>(
+        &'a mut self,


The 'a lifetime requirements in this signature are never used.

- pub fn mark_from<'a, TMarkedSource: IntoIterator<Item = &'a proc_macro2::Ident> + 'a>( - &'a mut self, + pub fn mark_from<'a, TMarkedSource: IntoIterator<Item = &'a proc_macro2::Ident>>( + &mut self,

Hum. I thought if I had one explicit (the one used in IntoIterator::Item) they were supposed to all be explicit. Neat.

Dessix · 2021-09-05T03:30:22Z

@dtolnay Awesome- your new version includes the sort of refactoring I was struggling to specify!

I believe you may be able to test an "upper bound" for these requirements such that superfluous ones cannot be introduced by use of a mechanism along the lines of the following "compile to prove" functions, by supplying expected-minimum-bounded generics as a maximum upper bound:

#[allow(unused)]
fn static_display_upper_bounds<TDisplayOnly, TDebugOnly, TNoFormat>(
    instance: &EnumCompound<TDisplayOnly, TDebugOnly, TNoFormat>,
    f: &mut std::fmt::Formatter<'_>,
) -> std::fmt::Result
where
    TDisplayOnly: std::fmt::Display,
    TDebugOnly: std::fmt::Debug,
{
    Display::fmt(&instance, f)
}

#[allow(unused)]
fn static_transparent_upper_bounds<T>(
    instance: &StructTransparentGeneric<T>,
    f: &mut std::fmt::Formatter<'_>,
) -> std::fmt::Result
where
    T: std::error::Error,
{
    std::error::Error::source(&instance);
    Display::fmt(&instance, f)
}

Initial rough-out for enum generic handling

8d2c7d4

Add visitor which marks provided generics by name in variants

fedb020

dtolnay reviewed Aug 28, 2021

View reviewed changes

Dessix added 2 commits August 28, 2021 19:54

Support #[from] usage parsing using GenericUsageVisitor

64b2a0d

Add support for generic structs

f9ca456

Dessix changed the title ~~Automatic Generic Constraints~~ Automatic Generic-Derived Qualifiers Aug 29, 2021

Dessix added 2 commits August 30, 2021 03:31

Fully functional. Format literal parsing is present yet atrocious.

28751e0

Merge remote-tracking branch 'origin/master' into thiserror-79-error-…

268352c

…constraint-detection

Dessix marked this pull request as ready for review August 30, 2021 10:37

Dessix added 2 commits August 30, 2021 03:46

Fix compatibility with rust 1.31.0 and 1.36.0

5de9589

Apparently matches is newer than Rust 1.36.0

377de48

dtolnay reviewed Sep 5, 2021

View reviewed changes

dtolnay closed this Sep 5, 2021

dtolnay reviewed Sep 5, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatic Generic-Derived Qualifiers #145

Automatic Generic-Derived Qualifiers #145

Dessix commented Aug 28, 2021 •

edited

Dessix commented Aug 28, 2021

dtolnay left a comment

Dessix commented Aug 28, 2021 •

edited

Dessix commented Aug 30, 2021

Dessix commented Sep 4, 2021

Dessix commented Sep 4, 2021 •

edited

dtolnay left a comment

dtolnay Sep 5, 2021

Dessix Sep 5, 2021 •

edited

dtolnay Sep 5, 2021

Dessix Sep 5, 2021

dtolnay Sep 5, 2021

Dessix Sep 5, 2021

dtolnay Sep 5, 2021

Dessix Sep 5, 2021

dtolnay Sep 5, 2021

Dessix Sep 5, 2021

dtolnay Sep 5, 2021

Dessix Sep 5, 2021

dtolnay Sep 5, 2021

dtolnay Sep 5, 2021

Dessix Sep 5, 2021

Dessix commented Sep 5, 2021

		pub fn mark_from<'a, TMarkedSource: IntoIterator<Item = &'a proc_macro2::Ident> + 'a>(
		&'a mut self,

Automatic Generic-Derived Qualifiers #145

Automatic Generic-Derived Qualifiers #145

Conversation

Dessix commented Aug 28, 2021 • edited

Dessix commented Aug 28, 2021

dtolnay left a comment

Choose a reason for hiding this comment

Dessix commented Aug 28, 2021 • edited

Dessix commented Aug 30, 2021

Dessix commented Sep 4, 2021

Dessix commented Sep 4, 2021 • edited

dtolnay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dessix Sep 5, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dessix commented Sep 5, 2021

Dessix commented Aug 28, 2021 •

edited

Dessix commented Aug 28, 2021 •

edited

Dessix commented Sep 4, 2021 •

edited

Dessix Sep 5, 2021 •

edited