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

Add the initial draft for inference-using-bounds #4140

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

chloestefantsova
Copy link
Contributor

This PR adds the initial draft for the inference-using-bounds language feature specification.

Copy link
Member

@stereotype441 stereotype441 left a comment

Choose a reason for hiding this comment

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

lgtm, but since I'm relatively inexperienced in writing specs, I'm going to tag in @eernstg as another reviewer

@kevmoo
Copy link
Member

kevmoo commented Oct 25, 2024

Cool to see the inline mermaid!

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

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

Looks good!

I think the actual feature specification is the modification of 'inference.md', and the other document is all about background and motivation.

So I'm suggesting that feature-specification.md is renamed (perhaps to design-document.md).

With this rename in place, it's perfectly fine that the document doesn't specify the precise rules, and that it is completely example-driven.

I do suggest, though, that the formal update (that is, the new text in inference.md) is mentioned up front in the design document, such that (1) readers will still know what the diff is without needing to track down the git update where inference.md was updated, and (2) the design document is explicit about the precise thing that it's motivating and explaining. This would just be a short section.

boundsDownwards --> csstvDownwards
boundsUpwards --> csstvUpwards
```

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps add a paragraph about the meaning of the subgraph X extends A -> X <: A<_> -> X <: A<_> in the upwards phase.

What's the intended meaning of being below the center line (like the first X <: A<_> node) or above (like the second one)? Why do we have two of them? Why is this information left unconnected to other parts of the process (is it ignored)?.

I'm pushing on this because we don't have too much documentation about how type inference works, and I think this is great! ;-) So let's make sure the reader can learn as much as possible from these graphs.

boundsUpwards --> csstvUpwards
```

## Proposed changes
Copy link
Member

Choose a reason for hiding this comment

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

I think the text should proceed to specify the semantics of this feature, rather than putting forward a proposal (we might then have several different proposals, or this one might change in various ways). That is, it should say "This is how it works" rather than "We propose ...".

The section title would then also need to be more firm. Perhaps:

Suggested change
## Proposed changes
## Rules
With this feature, the rules are modified as follows.

@@ -0,0 +1,319 @@
# Inference using bounds
Copy link
Member

Choose a reason for hiding this comment

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

Having looked at this PR as a whole, it is my impression that this document isn't a feature specification at all, it is all about background and motivation.

That's fine, and there's nothing wrong with a PR that changes the language by modifying an existing feature specification (namely resources/type-system/inference.md), and then adds motivational and background information in a new document (namely this one).

However, we should then probably rename this document (for instance, from feature-specification.md to design-document.md), and then add a section to this document (that might be just before Motivating example) indicating the precise change (that would be a copy of the lines 726-731 in resources/type-system/inference.md), plus just a few words saying that this is a new item in the specification of https://github.com/dart-lang/language/blob/main/resources/type-system/inference.md#constraint-solution-for-a-set-of-type-variables (and perhaps also https://github.com/dart-lang/language/blob/main/resources/type-system/inference.md#grounded-constraint-solution-for-a-set-of-type-variables?), and then something like "The rest of this document explains how this change improves type inference in Dart".

This means that we've put the hard facts away once and for all, and anyone who reads the design document in the future will know that (1) this change was actually an update to 'inference.md', and (2) the change was adding this item to the algorithm. This means that (3) they will be able to find that item in the actual (updated) inference.md in the future, and the information in this document will help them understand why this was done.

In particular, the information in this document can then be focused 100% on explaining examples and giving motivation, and there's no need to dive into the precise general rules.

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

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

Let me just do LGTM, to make sure I am not blocking anything.

@sgrekhov
Copy link
Contributor

sgrekhov commented Nov 1, 2024

Github doesn't allow to comment that line, so I'm writing it here. File inference.md, informal section on lines L745-746.

The bound of a type variable is only included as a constraint when the
choice of type for that type variable is about to be frozen.

Seems that it should be updated as well.

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

Successfully merging this pull request may close these issues.

5 participants