Do witness resolution & forwarding in Converter - #3049
Merged
Conversation
Contributor
Author
|
@nordlander I felt I had a bit of an epiphany during #3045, an insight upon which this branch is then based. simple and elegant 😃 the branch is actually a net-negative in terms of lines of code, removing more code than it adds (if we do not count the new test cases). It is also quite possible that I'm missing something central on how this needs to work, rendering the PR useless 😜 |
added 2 commits
August 17, 2026 08:41
Add a test fixture, witness_forward.act, with golden files for the converted AST (pass 3) and the generated C (pass 9). The fixture has a protocol PA with two methods, one static (same) and one instance method (total). Protocols PB and PC both inherit PA. Class Thing gets two extensions. The first, for PB, implements all the methods. The second, for PC, implements only PC's own method, because the methods of PA are already implemented by the first extension and may not be implemented again. The golden files record what the compiler produces today for the witness class of the second extension, PCD_Thing. In the converted AST, same and total are just signatures with no implementation. In the C code, CodeGen fills the two empty method table slots with $forward wrappers that call the corresponding methods of PBD_Thing. But the class is still treated as abstract, so no PCD_ThingG_new constructor is generated, and any program that needs to create a PCD_Thing witness fails to compile. The next commit changes both outputs.
When two protocols share an ancestor, the witness class for the
second one inherits method slots that are already implemented by the
first. Ord and Hashable both inherit Eq. Extensions are checked in
source order, and the first extension that covers a protocol must
implement its methods; a later extension covering the same protocol
may not implement them again. So with extension int (Ord) written
before extension int (Hashable), the Ord extension implements
__eq__, and the Hashable witness for int gets an __eq__ slot whose
implementation lives in the Ord witness. The type checker left such
slots empty and CodeGen filled them in: when emitting C, it searched
all classes for one with a matching method and generated a C wrapper
that calls it. That repair only existed in the C output. The
converted AST still had the empty slots, so every later pass had to
allow for that. And it only fixed the method table: the witness class
was still abstract, so no constructor was generated for it, and a
program that needed one failed to compile.
Now the type checker fills the slots itself when it checks the
extension. A slot is left empty exactly when its protocol is already
covered by an earlier witness (these are the final slots that
checkAttributes finds). For each such slot the extension body gets an
ordinary method that calls the same method through the protocol that
declares it:
def total(self): return Coll.total(self)
The type checker then resolves Coll.total to the witness that
implements it, exactly as it would for a call written by hand. That
also handles the cases the old search could not: providers reached
through a witness field of another witness (Collection through
Sequence$list.W_Collection), generic providers whose type arguments
must be instantiated, cyclic witnesses whose extra opts must be
padded, and providers with constructor arguments. Forwarding is
always a single hop, because the witness it reaches is the first one
covering the protocol, and that one implements the method directly.
If a slot is inherited from several parents it gets one forwarding
method, and the parents' signatures must agree; disagreeing
signatures are now an error instead of being silently resolved.
The forwarding call inside the extension body must resolve to the
earlier witness. While the body is checked, the extension's own
witnesses are in scope too, one per protocol in its ancestry, and the
one for the covered protocol would shadow the earlier witness the
call is meant to reach. So the visible-witness skip that keeps
duplicates out of the module table (hasVisibleWit) is now applied to
those self-witnesses as well (tydefineInst).
Witness classes come out of the type checker complete, so the
forwarding machinery in CodeGen is removed, along with the never
called provider search next to it and the hand-written C
implementations of builtin methods that the generated forwarding
methods now replace. The builtin method tables no longer contain any
$forward wrappers, and the generated __eq__ forwarders compile down
to plain value comparisons.
plajjan
force-pushed
the
converter-witness-forwarding
branch
from
August 17, 2026 07:40
70dd664 to
ad470e0
Compare
Contributor
|
This change is really good, I like it a lot! Very appropriate to let the type-checker itself sort out what witnesses to use in the forwarding methods. I'm just curious about the potential clashes that would yield the "Conflicting inherited signatures" error. Has an example of where this happens occurred? My intuition is that the mro algorithm would have trapped any such inconsistency already, but I could be wrong. |
Contributor
Author
|
@nordlander I am not aware of any "real" occurrences in real life - only this new test case introduced in this PR test/core_lang_auto/witness_forward_diamond_clash__bf.act which shows the problem and MRO does not seem to throw any error there. Rather than resolving the wrong thing, we explicitly throw an error. Makes sense? |
Contributor
|
Yes, makes sense. Good to have an example, I’ll check it up. But we should be good to merge.
|
Contributor
Author
|
@nordlander "should be" sounds like you still might have some last doubts ;) If you think we should merge, please press the merge button :) |
Contributor
|
Ok, I did some more digging on the MRO topic and decided to make a few additions/clarifications to the clash test while we're at it. Here's what I found:
Merging. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This builds on #3043 and #3045 (perhaps easier to review them individually), so it's just the last 2 commits in this PR that is the real change in this PR.
We currently have some code running in CodeGen to fill in unpopulated witness slots, like when a protocol inherits from some other protocol but a particular extension doesn't concretely specify all methods. Our classic example is in builtins where both the Ord and Hashable protocol inherit from Eq. Some instance implement Ord, which define
__eq__to cover the Eq part, and then the later Hashable protocol should have its__eq__resolved to the available method. The first extension to implement the methods for a protocol "owns" it and can be used by later extensions.Doing this in CodeGen is really rather late. For the work on selective back passes, we want to collect this information about witnesses and we can't do this from CodeGen, so we repeated the effort. This PR does a more proper move of this witness resolution functionality into Converter so it is available in the AST just after the type checking pass. The change itself is structured into 2 commits, the first which adds a snapshot test so we can see in the second commit the effect it has on the AST.