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.

Sign up for GitHub

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

Jump to bottom

Should Parser use associated types, rather than generics? #163

Closed
epage opened this issue Feb 15, 2023 · 4 comments
Closed

Should Parser use associated types, rather than generics? #163

epage opened this issue Feb 15, 2023 · 4 comments
Labels
C-question Uncertainty is involved M-breaking-change Meta: Implementing or merging this will introduce a breaking change.

Comments

@epage
Copy link
Collaborator

epage commented Feb 15, 2023

I experimented with this in #5 but my motivating case went away. nom went forward with this in rust-bakery/nom#1626. In rust-bakery/nom#1631, type inference worked better than in #6 for parsers returning concrete types because of the associated types.
@epage epage added M-breaking-change Meta: Implementing or merging this will introduce a breaking change. C-enhancement Category: Raise on the bar on expectations labels Feb 15, 2023
@epage epage added this to the 0.4 milestone Feb 15, 2023
@epage epage added C-question Uncertainty is involved and removed C-enhancement Category: Raise on the bar on expectations labels Feb 15, 2023
@epage epage mentioned this issue Feb 15, 2023
Closed
2 tasks
epage added a commit to epage/winnow that referenced this issue Mar 21, 2023
@epage
WIP: fix!: Use associated types for improve inference
b600244 
This is a re-implementation of winnow-rs#5.  It was abandoned before as we didn't
have a motivating case.  Now, we've seen it can improve type inference,
so giving it another try.

Fixes winnow-rs#163
@epage epage mentioned this issue Mar 21, 2023
Closed
epage added a commit to epage/winnow that referenced this issue Mar 21, 2023
@epage
WIP: fix!: Use associated types for improve inference
51e3156 
This is a re-implementation of winnow-rs#5.  It was abandoned before as we didn't
have a motivating case.  Now, we've seen it can improve type inference,
so giving it another try.

Fixes winnow-rs#163
@epage
Copy link
Collaborator Author

epage commented Mar 21, 2023

With #220, it looks like associated types make it so we can't imp Parser for built-ins because there is nothing to constrain E on and we don't want to hard code it.

@epage
Copy link
Collaborator Author

epage commented Mar 21, 2023

We could limit this to just Output but I'm worried about the side effects of that inconsistency

@epage epage removed this from the 0.4 milestone Mar 21, 2023
epage added a commit to epage/winnow that referenced this issue Mar 22, 2023
@epage
fix!: Don't lose type information on Parser functions
29f6a06 
When playing around with winnow-rs#163, I found that carrying more type
information along in `Parser`s, type inference works better, offering an
alternative to solving the core problem that associated types was
looking to solve.
@epage epage mentioned this issue Mar 22, 2023
Merged
epage added a commit to epage/winnow that referenced this issue Mar 22, 2023
@epage
fix!: Don't lose type information on Parser functions
cc1b363 
When playing around with winnow-rs#163, I found that carrying more type
information along in `Parser`s, type inference works better, offering an
alternative to solving the core problem that associated types was
looking to solve.
epage added a commit to epage/winnow that referenced this issue Mar 22, 2023
@epage
fix!: Don't lose type information on Parser functions
d32b03a 
When playing around with winnow-rs#163, I found that carrying more type
information along in `Parser`s, type inference works better, offering an
alternative to solving the core problem that associated types was
looking to solve.
@epage
Copy link
Collaborator Author

epage commented Mar 31, 2023

The driving benefit for this was resolved in #222 using more phantom data. afaik the main benefit for associated types at this point is removing the phantom data.

@epage
Copy link
Collaborator Author

epage commented Apr 27, 2023

With how that failed, I'm closing this

@epage epage closed this as not planned Won't fix, can't repro, duplicate, stale Apr 27, 2023
@epage epage mentioned this issue Nov 17, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-question Uncertainty is involved M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

WIP: fix!: Use associated types for improve inference epage/winnow
1 participant
@epage