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

Associated constant assignment versus equality #2173

Merged
merged 19 commits into from
Feb 1, 2023

Conversation

zygoloid
Copy link
Contributor

@zygoloid zygoloid commented Sep 13, 2022

Split the where A == B constraint in two: where .A = B produces a new
constraint from an old one, where the value of .A in the new constraint is
known and eagerly rewritten to B, and where A == B, which does not cause
A and B to be considered as identical by language rules but does permit
implicit (no-op) conversion between them.

This aims to provide an efficiently-computable and human-understandable type
equality rule, with type canonicalization and therefore transitive type
equality, without sacrificing too much in the way of ergonomics and without
sacrificing determinism, while still providing the full power of a general type
constraint system in a less ergonomic form.

@zygoloid zygoloid added proposal A proposal proposal draft Proposal in draft, not ready for review labels Sep 13, 2022
@zygoloid zygoloid changed the title Associated constraint assignment versus equality Associated constant assignment versus equality Sep 13, 2022
@zygoloid zygoloid marked this pull request as ready for review September 13, 2022 21:35
@zygoloid zygoloid removed the proposal draft Proposal in draft, not ready for review label Sep 13, 2022
@github-actions github-actions bot added the proposal rfc Proposal with request-for-comment sent out label Sep 13, 2022
Copy link
Contributor

@josh11b josh11b left a comment

Choose a reason for hiding this comment

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

Here are some initial comments. I'm still reading and pondering, so I may have more later.

proposals/p2173.md Show resolved Hide resolved
proposals/p2173.md Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Show resolved Hide resolved
Co-authored-by: josh11b <[email protected]>
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Show resolved Hide resolved
Copy link
Contributor

@josh11b josh11b left a comment

Choose a reason for hiding this comment

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

I've tried to resolve stuff so it is clear what is left.

proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Show resolved Hide resolved
Remove the "recursively uses the same rewrite" rule which appears to be
unnecessary to guarantee termination.
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Show resolved Hide resolved
proposals/p2173.md Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
jonmeow pushed a commit to jonmeow/carbon-lang that referenced this pull request Oct 12, 2022
Implements basic support for rewrite constraints as proposed in carbon-language#2173.

Support for associated constants and complex constraints in general is also made more robust: argument deduction now properly computes and substitutes witnesses, and interface declarations track a more correct description of the constraint that they introduce than the one we previously built.
zygoloid added a commit that referenced this pull request Oct 18, 2022
…sfied (#2294)

Add checking that a type only satisfies a constraint if it satisfies all of that constraint's equality and rewrite constraints.

Enforce the rule that `=` must be used in impls when specifying associated constant values rather than `==`.

Turn off the pre-#2173 single-step equality behavior. This is getting somewhat ahead of the approved design, but it's a one-line change to restore the old behavior.

Fix `CARBON_CHECK` to handle top-level `,`s in its argument, such as may happen in template argument lists, as this change introduces such a check.
zygoloid added a commit that referenced this pull request Oct 24, 2022
)

Instead of checking and resolving rewrites eagerly, defer doing so until a constraint is applied to a binding. Actual resolution of rewrites is not yet implemented, so this is mostly just refactoring, but the eventual goal is to support things like `(Constraint where .T = .U) & (Constraint where .U = V)` (however they are reached) by applying one rewrite to the other, as agreed with @josh11b in discussion of #2173.

One nice consequence is that substitution into a constraint type no longer has a failure path, so substitution is once again not able to fail.
Copy link
Contributor Author

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

@josh11b This has been substantially revised based on implementation experience and a switch to resolving rewrites late rather than immediately. I would appreciate any further feedback!

Copy link
Contributor

@josh11b josh11b left a comment

Choose a reason for hiding this comment

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

I think this is really close, I'm very excited!

proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

Two real things I think are still needed here:

  1. Updating to the new terminology direction, and
  2. Clarifying (and maybe tweaking) the impl as constraints.

I'm suggesting (1) here even in the proposal because I suspect this particular proposal and write-up will end up fairly heavily read and cited, so it seems worthwhile to align it to the newer terminology and stuff like type to avoid confusion in the future.

proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
rule cannot be readily understood by a developer nor effectively worked
around.
- Rust also has an
[undecidable type system (content warning: swear words)](https://sdleffler.github.io/RustTypeSystemTuringComplete/)
Copy link
Contributor

Choose a reason for hiding this comment

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

Trivial nit: slightly more precise / clear CW?

Suggested change
[undecidable type system (content warning: swear words)](https://sdleffler.github.io/RustTypeSystemTuringComplete/)
[undecidable type system (content warning: profanity)](https://sdleffler.github.io/RustTypeSystemTuringComplete/)

Also, wow, this is such a rabbit hole of esoteric languages...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to push back on this suggestion just a little: in this context, the word in question is not being used as profanity but simply as part of a proper noun, but it is nonetheless objectively a swear word. Happy to accept the suggestion if you still think it's a beneficial change given that argument.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your point there.

I struggle with "swear words" because for me it seems like a slang term or idiom and doesn't unambiguously communicate the severity of the word choice. A result is that I'm not sure it'll serve its purpose reliably.

Maybe "obscenities" or "obscene words" or "obscene language" would work to avoid talking about the usage but still make the warning a bit more pointed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried a rewrite to make this clearer, at the expense of being a lot more verbose :)

@chandlerc
Copy link
Contributor

IS this ready for another round of review? (It wasn't marked as such, but looks like it might be...)

@zygoloid
Copy link
Contributor Author

IS this ready for another round of review? (It wasn't marked as such, but looks like it might be...)

Yes. What kind of marking are you looking for?

Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

I think this is in great shape. One comment below is just a discussion topic I wrote down to avoid forgetting. The rest I think are super minor. If they all make sense, don't block, but feel free to ping for a quick look. I don't tihnk anything left needs re-reading significant sections.

Also, this is a fantastic write-up and I'm really excited about the whole thing. It's a remarkably complex problem and area, and the treatment here I think is great.

Comment on lines +174 to +175
import – and changing the type, such as by writing a different expression after
a `:` or `:!`. Changing the type of a value is expected to be able to change the
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, with the new terminology, a different expression in type position doesn't change the type of the value any more? Guessing this is just another place needing a terminology update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I mean here is that changing from a: i32 to a: f32 changes the type of a, and changing from T:! C to T:! C where .X = 5 changes the type of T. I think what you're observing is that changing from T:! C = i32 to T:! (C where .X = 5) = i32 doesn't change the type of a: T -- either way, it's a: i32, because the right-hand side of : is converted to type.

Is there some way I can update this paragraph to make it clearer what I'm talking about?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of anything.

The root issue is "writing a different expression after a : or :!." -> we've just recently allowed that to not always change the type, so I get a little surprised when that is the first example of how to change the type of the value. But of course, it is, and different expressions that don't change the type of the value are the exception and not the rule. So I think its ultimately fine. =D

proposals/p2173.md Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
rule cannot be readily understood by a developer nor effectively worked
around.
- Rust also has an
[undecidable type system (content warning: swear words)](https://sdleffler.github.io/RustTypeSystemTuringComplete/)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see your point there.

I struggle with "swear words" because for me it seems like a slang term or idiom and doesn't unambiguously communicate the severity of the word choice. A result is that I'm not sure it'll serve its purpose reliably.

Maybe "obscenities" or "obscene words" or "obscene language" would work to avoid talking about the usage but still make the warning a bit more pointed?

proposals/p2173.md Outdated Show resolved Hide resolved
proposals/p2173.md Outdated Show resolved Hide resolved
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

Ship it!

Comment on lines +174 to +175
import – and changing the type, such as by writing a different expression after
a `:` or `:!`. Changing the type of a value is expected to be able to change the
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of anything.

The root issue is "writing a different expression after a : or :!." -> we've just recently allowed that to not always change the type, so I get a little surprised when that is the first example of how to change the type of the value. But of course, it is, and different expressions that don't change the type of the value are the exception and not the rule. So I think its ultimately fine. =D

@zygoloid zygoloid merged commit 0b6411e into carbon-language:trunk Feb 1, 2023
github-merge-queue bot pushed a commit that referenced this pull request Sep 19, 2023
First step in updating `docs/design/generics/details.md`. It
incorporates changes from proposals: #989 #2138 #2173 #2200 #2360 #2964
#3162 , but there are still more changes from those proposals to be
made.

It also switches away from suggesting static-dispatch witness tables,
and creates an appendix to describe that decision.

---------

Co-authored-by: Richard Smith <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Oct 3, 2023
Continued from part 1: #3231. Second step updating
`docs/design/generics/details.md`. There remains some work to
incorporate proposal #2200.

- The biggest changes are incorporating much of the text of proposals:
  - #2173
  - #2687
- It incorporates changes from proposals:
  - #989
  - #1178
  - #2138
  - #2200
  - #2360
  - #2964
  - #3162
- It also updates the text to reflect the latest thinking from leads
issues:
  - #996
  - #2153 -- most notably deleting the section on `TypeId`.
- Update to rule for prioritization blocks with mixed type structures
from [discussion on
2023-07-18](https://docs.google.com/document/d/1gnJBTfY81fZYvI_QXjwKk1uQHYBNHGqRLI2BS_cYYNQ/edit?resourcekey=0-ql1Q1WvTcDvhycf8LbA9DQ#heading=h.7jxges9ojgy3)
- Adds reference links to proposals, issues, and discussions relevant to
the text.
- Also tries to use more precise language when talking about
implementations, to avoid confusing `impl` declaration and definitions
with the `impls` operator used in `where` clauses, an issue brought up
in
  - #2495 
  - #2483

---------

Co-authored-by: Richard Smith <[email protected]>
@zygoloid zygoloid deleted the proposal-assoc-const-where branch October 4, 2023 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal rfc Proposal with request-for-comment sent out proposal A proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants