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

spec: allow record {} <: record {null} #462

Merged
merged 4 commits into from
Aug 30, 2023
Merged

spec: allow record {} <: record {null} #462

merged 4 commits into from
Aug 30, 2023

Conversation

chenyan-dfinity
Copy link
Contributor

@chenyan-dfinity chenyan-dfinity commented Aug 27, 2023

In the current spec, we only allow record {} <: record {opt t; reserved}, i.e. users can omit opt fields or reserved fields in the argument, but cannot omit null fields.

In the Rust implementation, however, we accidentally allowed null fields to be optional as well. And this bug has become a feature that people start to use it in the wild...The most prominent use is to decode an empty blob DIDL\0\0 into a null value, which corresponds to () in Rust.

This PR tries to make that bug into a feature to allow optional null fields in the record.

Another option is to make () in Rust map to empty message in Candid, and have a dedicated candid::Null type in Rust. But that's also a breaking change.

@chenyan-dfinity
Copy link
Contributor Author

cc @rossberg

@github-actions
Copy link

Benchmark for 84ebd6e

Click to view benchmark
Test Base PR %
Blob/&str 289.9±84.60µs 274.4±81.72µs -5.35%
Blob/ByteBuf 191.1±55.16µs 197.2±69.98µs +3.19%
Blob/Bytes 123.7±50.92µs 123.0±50.76µs -0.57%
Blob/String 358.1±172.13µs 358.0±172.81µs -0.03%
Collections/vec (text, nat) 80.6±1.10ms 82.2±1.46ms +1.99%
Collections/vec int 38.9±0.12ms 39.0±0.12ms +0.26%
Collections/vec int64 21.4±0.39ms 21.9±0.45ms +2.34%
Collections/vec nat8 17.5±0.96ms 17.2±0.04ms -1.71%
option list/1024 1599.3±1.41µs 1603.7±4.79µs +0.28%
profiles/1024 3.2±0.01ms 3.3±0.05ms +3.13%
variant list/1024 1293.1±1.25µs 1293.4±2.35µs +0.02%

@github-actions
Copy link

Benchmark for d44bbcb

Click to view benchmark
Test Base PR %
Blob/&str 190.4±66.48µs 189.8±64.85µs -0.32%
Blob/ByteBuf 134.6±49.61µs 134.5±49.09µs -0.07%
Blob/Bytes 95.1±33.55µs 97.0±36.22µs +2.00%
Blob/String 208.4±75.20µs 211.5±73.67µs +1.49%
Collections/vec (text, nat) 61.6±0.73ms 61.3±0.66ms -0.49%
Collections/vec int 30.0±0.09ms 30.2±0.05ms +0.67%
Collections/vec int64 17.1±0.10ms 17.3±0.16ms +1.17%
Collections/vec nat8 13.2±0.07ms 13.4±0.06ms +1.52%
option list/1024 1333.1±1.61µs 1341.5±1.28µs +0.63%
profiles/1024 2.6±0.00ms 2.6±0.01ms 0.00%
variant list/1024 1131.1±1.82µs 1131.7±3.25µs +0.05%

@crusso
Copy link
Contributor

crusso commented Aug 28, 2023

I'm not opposed - might even be more consistent with subtyping since nulls are options too, but curious what @nomeata and @rossberg think.

Would need a new version of Candid I think. And updating of Motoko unless it has the same behaviour already.

@nomeata
Copy link
Collaborator

nomeata commented Aug 28, 2023

I vaguely remember that we discussed this before, and I believe I was in favor. Maybe someone finds the conversation.

@crusso
Copy link
Contributor

crusso commented Aug 28, 2023

I vaguely remember that we discussed this before, and I believe I was in favor. Maybe someone finds the conversation.

I couldn't find it on my mobile. So didn't look that hard

Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

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

LGTM (on mobile)

rust/candid/src/de.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@ggreif ggreif 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, comments added.

@github-actions
Copy link

Benchmark for d23c920

Click to view benchmark
Test Base PR %
Blob/&str 244.6±72.04µs 243.1±71.51µs -0.61%
Blob/ByteBuf 156.4±45.90µs 160.3±44.01µs +2.49%
Blob/Bytes 109.5±31.55µs 109.4±33.12µs -0.09%
Blob/String 245.4±70.83µs 245.6±70.17µs +0.08%
Collections/vec (text, nat) 69.5±0.85ms 69.3±1.13ms -0.29%
Collections/vec int 32.4±0.08ms 31.8±0.11ms -1.85%
Collections/vec int64 18.7±0.36ms 18.5±0.28ms -1.07%
Collections/vec nat8 13.8±0.02ms 13.8±0.03ms 0.00%
option list/1024 1383.3±6.83µs 1377.7±5.73µs -0.40%
profiles/1024 2.7±0.05ms 2.7±0.01ms 0.00%
variant list/1024 1139.3±5.51µs 1133.9±2.35µs -0.47%

@github-actions
Copy link

Benchmark for b18531e

Click to view benchmark
Test Base PR %
Blob/&str 283.3±85.67µs 283.7±83.56µs +0.14%
Blob/ByteBuf 182.6±53.63µs 183.9±54.11µs +0.71%
Blob/Bytes 126.5±35.47µs 121.5±36.31µs -3.95%
Blob/String 310.9±95.37µs 310.7±94.50µs -0.06%
Collections/vec (text, nat) 82.9±1.10ms 83.3±1.27ms +0.48%
Collections/vec int 38.8±0.08ms 38.6±0.69ms -0.52%
Collections/vec int64 23.1±0.22ms 22.3±0.28ms -3.46%
Collections/vec nat8 16.6±0.03ms 16.9±0.13ms +1.81%
option list/1024 1606.1±3.07µs 1618.9±10.06µs +0.80%
profiles/1024 3.3±0.01ms 3.2±0.01ms -3.03%
variant list/1024 1297.3±1.59µs 1302.7±10.11µs +0.42%

@chenyan-dfinity chenyan-dfinity merged commit 9f567c3 into master Aug 30, 2023
3 checks passed
@chenyan-dfinity chenyan-dfinity deleted the spec-null branch August 30, 2023 01:32
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.

4 participants