Skip to content

feat(rdl): add property and event shorthand syntax for interfaces#4143

Merged
kennykerr merged 2 commits intomasterfrom
copilot/add-simpler-syntax-patterns
Apr 8, 2026
Merged

feat(rdl): add property and event shorthand syntax for interfaces#4143
kennykerr merged 2 commits intomasterfrom
copilot/add-simpler-syntax-patterns

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 6, 2026

  • Identify build errors: garbled InterfaceMember::Method arm — WinRT type-validation and param code displaced into the wrong loops by a bad merge squash
  • Fix the corrupted Method arm in reader/interface.rs (restore WinRT param/return validation in correct positions, remove displaced param(pt) from attrs loop, remove dangling code outside the match arm)
  • Fix corrupted panic.rs test file (two new test functions missing their closing ); and })
  • All 54 panic tests + all rdl roundtrip/unit tests pass; clippy clean

@kennykerr
Copy link
Copy Markdown
Collaborator

@copilot this PR has unrelated changes - please rebase on master

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 6, 2026

@copilot this PR has unrelated changes - please rebase on master

The feature implementation is correct and only touches 7 files relative to current master (1ec3db9) — verified with git diff 1ec3db9..HEAD --stat.

The PR is showing unrelated files (25 total) because the stored PR base sha is 948deee (an older master commit from when this PR was first created), causing GitHub to compare our branch against that older baseline. The three unrelated master PRs (#4141, #4142, #4145) merged between then and now are showing up as part of our diff.

The fix requires force-pushing a clean linear branch (1ec3db9 → our-feature-commit) to replace the current merge-commit history. This is not possible through the automated tooling. If you can force-push the branch or trigger GitHub's "Update branch" + rebase, the PR will show only the correct 7 files. Alternatively, "Squash and merge" when ready to land will produce a single clean commit on master regardless of the branch history.

@kennykerr
Copy link
Copy Markdown
Collaborator

@copilot I can clone the branch - give me the exact git command to run.,

@kennykerr kennykerr marked this pull request as ready for review April 6, 2026 23:34
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 6, 2026

@copilot I can clone the branch - give me the exact git command to run.,

git clone https://github.com/microsoft/windows-rs
cd windows-rs
git fetch origin master copilot/add-simpler-syntax-patterns
git checkout copilot/add-simpler-syntax-patterns
git reset --soft 1ec3db9
git commit -m "feat(rdl): add property and event shorthand syntax for interfaces"
git push --force origin copilot/add-simpler-syntax-patterns

1ec3db9 is the current master tip. The reset --soft collapses the entire branch to a single clean commit on top of master without touching working tree or index.

@kennykerr kennykerr force-pushed the copilot/add-simpler-syntax-patterns branch from 11fd640 to d7cdb6e Compare April 7, 2026 00:18
@kennykerr
Copy link
Copy Markdown
Collaborator

@tim-weis thoughts?

@kennykerr
Copy link
Copy Markdown
Collaborator

@copilot - looks like a bad merge - please fix

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 7, 2026

@copilot - looks like a bad merge - please fix

The automated push tooling rebases rather than force-pushes, so it can't rewrite the merge history. To squash the bad merge down to a single clean commit on top of current master, run:

git fetch origin copilot/add-simpler-syntax-patterns
git checkout copilot/add-simpler-syntax-patterns
git reset --soft 0c719dc
git commit -m "feat(rdl): add property and event shorthand syntax for interfaces"
git push --force origin copilot/add-simpler-syntax-patterns

0c719dc is the current master tip. After this the branch will have exactly the 7 rdl feature files as a single commit on top of master.

@kennykerr kennykerr force-pushed the copilot/add-simpler-syntax-patterns branch from 9bd3766 to b7202ee Compare April 7, 2026 19:05
@kennykerr kennykerr merged commit 8d9839a into master Apr 8, 2026
30 checks passed
@kennykerr kennykerr deleted the copilot/add-simpler-syntax-patterns branch April 8, 2026 13:55
@tim-weis
Copy link
Copy Markdown
Contributor

tim-weis commented Apr 9, 2026

thoughts?

That looked good until it was brought to my attention that I had overlooked the opportunity for a collision with the RDL event keyword. The prospect of two competing syntaxes for describing properties in WinRT and COM isn't quite enticing. Going forward, either of these options is probably better:

  • Find a way to support the property shorthand syntax for COM interfaces, or
  • roll back to the more verbose, keyword-prefixed syntax.

Regardless of the approach taken, there are a few rough edges that should be addressed:

Canonical representation (QoL)

Enforcing canonical representations is a good thing, and I was delighted to see the RDL parser rejecting #[get] #[set] foo: u32; as a needlessly verbose version of foo: u32;. The following RDL is equally redundant, yet accepted:

#[winrt]
mod Test {
    interface ITest {
        #[get]
        foo: u32;
        #[set]
        foo: u32;
    }
}

This should be rejected for the same reason that #[get] #[set] ... is. This is purely a QoL improvement; the RDL round-trips with full semantic fidelity, just not lexically. Here's the relevant portion of what the Reader/Writer makes of it:

    interface ITest {
        foo: u32;
    }

Type loss (bug)

A similarly structured RDL surfaces a genuine bug:

#[winrt]
mod Test {
    interface ITest {
        #[get]
        foo: u32;
        #[set]
        foo: String; // <-- foo is of type String, not u32
    }
}

Running this RDL through the same Reader/Writer-cycle produces the same RDL output as the previous test (foo: u32;), dropping the type information for the #[set]-attributed property of the same symbol foo. This needs to be addressed.

V-table ordering (bug)

Toying about with this, I discovered another, more realistic bug: Interleaving disparate #[get] and #[set] property descriptions with unrelated entities coerces the getter/setter into consecutive v-table slots. As an example:

#[winrt]
mod Test {
    interface ITest {
        #[get]
        foo: u32;
        bar: u32;
        #[set]
        foo: u32;
    }
}

gets round-tripped into:

#[winrt]
mod Test {
    interface ITest {
        foo: u32;
        bar: u32;
    }
}

This effectively moves put_foo() in v-table-order ahead of get_bar(), breaking the ABI contract. The v-table entries should instead be: get_foo(), get_bar(), put_bar(), put_foo().

Symbol duplication (QoL/bug?)

There's another unrelated yet prevalent issue that's probably not exclusive to properties: Symbols can be repeated within the same scope. Consider the following RDL

#[winrt]
mod Test {
    interface ITest {
        foo: u32;
        foo: u32;
    }
}

This round-trips perfectly and makes for a valid interface description in terms of the ABI. This interface description, however, cannot be directly/easily projected into any programming language I'm familiar with. This is an issue that needs to be addressed.

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.

3 participants