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

Specify that dictionaries can't be the type of attr/constant #940

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

saschanaz
Copy link
Member

@saschanaz saschanaz commented Nov 22, 2020

The same sentence is already in Dictionary section, and this change copy-pastes it to the Types - Dictionary types for consistency with other Types sections.

Closes #938


Preview | Diff

@TimothyGu
Copy link
Member

It looks like the same could be done for callback function types, which cannot be the type of any constant.

@@ -5595,8 +5595,7 @@ and the return type and argument list (matching <emu-nt><a href="#prod-Type">Typ
and <emu-nt><a href="#prod-ArgumentList">ArgumentList</a></emu-nt>) on the right side of the equals
sign gives the signature of the callback function type.

[=Callback functions=] must not
be used as the type of a [=constant=].
[=Callback functions=] must not be used as the type of a [=constant=].
Copy link
Member

Choose a reason for hiding this comment

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

This duplicates the statement on line 6367. I think this should be removed, as the sentence is better fit in the types section.

Copy link
Member

Choose a reason for hiding this comment

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

Wait never mind, I misunderstood the PR.

Copy link
Member

Choose a reason for hiding this comment

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

Can someone explain this to me? The statements appear identical. How are they different?

Copy link
Member Author

Choose a reason for hiding this comment

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

this change copy-pastes it to the Types - Dictionary types for consistency with other Types sections.

I guess this?

Copy link
Member

Choose a reason for hiding this comment

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

So why do we only have the requirement on dictionaries once?

But also, we shouldn't have duplicate requirements. That needs a follow-up issue. (Preserving consistency for now seems okay.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding duplicates and then removing them sounds weird, maybe close this and file an issue to remove the existing ones?

Copy link
Member Author

Choose a reason for hiding this comment

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

(Ah no, the callback function thing is new, please don't just close this)

@bathos
Copy link
Contributor

bathos commented Oct 13, 2021

Given the description of consts says:

The type of a constant (matching ConstType) must not be any type other than a primitive type. If an identifier is used, it must reference a typedef whose type is a primitive type.

What is the purpose of these changes with regard to consts?

There are more types which cannot be the type of a constant (object, symbol, DOMString, ByteString, USVString, FrozenArray, ObservableArray, ArrayBuffer, DataView, Int32Array, ...) than the handful to which the new "can't be used as..." text is added. What is the distinction being made - why is it important to repeat the constraint that sequence cannot be used but not repeat that FrozenArray or Promise cannot be?

@saschanaz
Copy link
Member Author

For, again, consistency? I'm open to just remove the "this type can't be used in..." and prefer "the following types can't be used here... ".

@bathos
Copy link
Contributor

bathos commented Oct 13, 2021

Hm, what I'm wondering is why would e.g. callback interface not include "must not be used as the type of a constant" if these others do? Isn't it less consistent? Is there something special about the cases where this is being added?

@saschanaz
Copy link
Member Author

Ah, in that case I think it's just accidentally omitted. 👀

@bathos
Copy link
Contributor

bathos commented Oct 14, 2021

Just a recap so it's more clear what seems odd to me - please correct me if this analysis is incorrect:

  • There are twenty one sections for types which cannot be the type of a constant (as established by the section that defines constants).
  • Currently only one of those sections (callback functions) explicitly states that it cannot be the type of a constant. (It's not clear to me why this one was previously singled out.)
  • This PR would add the explicit "cannot be the type of a constant" to three more (dictionaries, sequences, records).
  • That will leave seventeen other type sections that continue to not say "cannot be the type of a constant" even though that would also be true for them.

@saschanaz
Copy link
Member Author

This PR would add the explicit "cannot be the type of a constant" to three more (dictionaries, sequences, records).

This PR just re-wraps the existing text for them, so they are not additions.

That said, I see only 5 matches of "must not be used as the type of" in the current spec, which does show there is some inconsistency.

@bathos
Copy link
Contributor

bathos commented Oct 14, 2021

Ah, got it. Sorry for the confusing comments. I was searching with an overspecific string. I see now that the 19 vs four (I think the fifth stems from appearing in relation callback function twice?) are a pre-existing quirk.

@annevk
Copy link
Member

annevk commented Oct 14, 2021

Okay, so principal requirement is this:

The type of a constant (matching ConstType) must not be any type other than a primitive type. If an identifier is used, it must reference a typedef whose type is a primitive type.

Right? And this one:

The type of the attribute, after resolving typedefs, must not be a nullable or non-nullable version of any of the following types:

  • a sequence type
  • a dictionary type
  • ...

I think that gives us two options for the sections on individual types:

  • We don't say anything.
  • We have notes with a statements of fact. "Note: can/cannot be used as a type of attribute/constant/???."

The latter option seems good, but would be a lot of additional work and if we want to do it consistently across all the various things, even more work.

I also think that means that this PR title is slightly misleading as it's already specified currently per those requirements I quoted. The problem is that some of these requirements are duplicated in other sections and others are not. I think for now the best course of action would be to remove the duplicate requirements, but if someone wants to add statements of fact to all types I won't stop them.

Hope that helps.

@saschanaz
Copy link
Member Author

I think the first option is simpler and gives us a single source of trust for each. I'll go ahead and do that if no one disagrees.

@bathos
Copy link
Contributor

bathos commented Oct 15, 2021

Meta aside, not specific to this but I think this discussion is an example of it:

It seems like duplicate (but not nec "complete") statements of requirements and constraints is a repeated issue in the spec currently and in some cases it's unclear where to look for the normative statement. The place where I've run into this most is extended attributes, where sections for various constructs state which extended attributes are applicable and then sections for extended attributes repeat this in a different way ... but they do not always say the same things, e.g. if you went by the former, [Unscopable] applies to no constructs at all.

@annevk
Copy link
Member

annevk commented Oct 15, 2021

@bathos could you file a follow-up issue on that? It sounds like a good first step there would also be to first go back to a single place (and ensure that is correct) and then perhaps later opt for annotations in other sections. And perhaps we should take this as a sign that if we duplicate the information again there should be more editor guidance in the source to ensure the various bits are kept synchronized (or we try to automate it somehow).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Why are dictionaries disallowed for attribute types?
4 participants