webidl icon indicating copy to clipboard operation
webidl copied to clipboard

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

Open saschanaz opened this issue 5 years ago • 12 comments

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

saschanaz avatar Nov 22 '20 13:11 saschanaz

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

TimothyGu avatar Jan 07 '21 23:01 TimothyGu

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?

bathos avatar Oct 13 '21 16:10 bathos

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... ".

saschanaz avatar Oct 13 '21 16:10 saschanaz

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?

bathos avatar Oct 13 '21 17:10 bathos

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

saschanaz avatar Oct 13 '21 17:10 saschanaz

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.

bathos avatar Oct 14 '21 00:10 bathos

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.

saschanaz avatar Oct 14 '21 00:10 saschanaz

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.

bathos avatar Oct 14 '21 00:10 bathos

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.

annevk avatar Oct 14 '21 07:10 annevk

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.

saschanaz avatar Oct 14 '21 12:10 saschanaz

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.

bathos avatar Oct 15 '21 02:10 bathos

@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).

annevk avatar Oct 15 '21 06:10 annevk