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

assertKeyPattern(M.kind("null")) fails with "null keys are not supported") #1489

Closed
gibson042 opened this issue Feb 7, 2023 · 6 comments · Fixed by Agoric/agoric-sdk#7035
Assignees

Comments

@gibson042
Copy link
Contributor

I observed when exploring Agoric/agoric-sdk#6903 (comment) that a 'null' case appears to be missing from matchKindHelper's checkKeyPattern (e.g., matches(undefined, M.kind("undefined")) and matches(null, M.kind("null")) are both true, but assertKeyPattern(M.kind("undefined")) passes while assertKeyPattern(M.kind("null")) fails with "null keys are not supported"), but then realized that kind has no documentation and its purpose isn't quite clear. Do we really want to be conflating passStyleOf results with tagged record tags, and if so then which of them count as "key" patterns and which do not?

@erights
Copy link
Contributor

erights commented Feb 8, 2023

First, on the narrow issue:

"null keys are not supported"

Looks like a bug to me.

@FUDCo ,
IIRC, assertKeyPattern was added to support something you were doing in collectionManager.js . Do you remember what this was about, and if 'null' was excluded on purpose? Would anything break if we stopped excluding null?

@erights
Copy link
Contributor

erights commented Feb 8, 2023

On the broader issue:

Do we really want to be conflating passStyleOf results with tagged record tags, and if so then which of them count as "key" patterns and which do not?

I initially felt very uncomfortable with this, and included a TODO or similar comment warning that we really need to revisit this mixing of namespaces. Since then, I have become more comfortable with it for reasons I can explain, and I know of no alternative I'd like better. We should certainly talk about it, and see if we can invent something better that I missed.

@erights
Copy link
Contributor

erights commented Feb 8, 2023

The name "passStyleOrRecordTag" should certainly be changed, as it no longer reflects how I think of the kind taxonomy.

@erights
Copy link
Contributor

erights commented Feb 9, 2023

See Agoric/agoric-sdk#6953

@erights erights self-assigned this Feb 9, 2023
@erights erights changed the title What is the purpose of Matcher M.kind(passStyleOrRecordTag) assertKeyPattern(M.kind("null")) fails with "null keys are not supported") Feb 19, 2023
@erights
Copy link
Contributor

erights commented Feb 19, 2023

There is no passStyleOrRecordTag. Renaming this bug to the problem being reported.

But @gibson042 , I owe you an explanation of the "kind" namespace, and why I became comfortable with it. Let's talk.

@erights
Copy link
Contributor

erights commented Feb 27, 2023

Fixed by Agoric/agoric-sdk#7035

@erights erights closed this as completed Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants