Validate optionality of dictionary arguments

[foolip]

This is intended to catch issues like the one in w3c/performance-timeline#119, fixed by w3c/performance-timeline#120.

Here's a snippet of code that should cause validation errors:

const { parse, validate } = require("webidl2");
const tree = parse(`
dictionary BrilliantInit {
  any stuff;
};

[Exposed=Window]
interface X {
    constructor(BrilliantInit dict);
};
`);
const validation = validate(tree);
console.log(validation);

The dict argument there should be optional.

It might also be a good idea to ensure that if a dictionary has required members, then it's not optional. However, https://heycam.github.io/webidl/#idl-operations doesn't seem to require this...

@bzbarsky

[bzbarsky]

It might also be a good idea to ensure that if a dictionary has required members, then it's not optional

Yeah, I've thought about that. In theory one could suppose an API that has an optional dictionary which, if present, must have some members set. But in practice, it seems like it then assumes some values for those members if the dictionary is not present, so you can express it as an optional dictionary with default values...

I filed whatwg/webidl#793 to track that.

[foolip]

@saschanaz I'm guessing a fix for this would be here?

webidl2.js/lib/productions/argument.js

Lines 50 to 58 in 1699329

	if (this.idlType.nullable) {
	const message = `Dictionary arguments cannot be nullable.`;
	yield validationError(this.tokens.name, this, message);
	} else if (this.optional && !this.default) {
	const message = `Optional dictionary arguments must have a default value of \`{}\`.`;
	yield validationError(this.tokens.name, this, message, {
	autofix: autofixOptionalDictionaryDefaultValue(this)
	});
	}

However, what behavior do you want when a dictionary inherits from another and the original dictionary isn't defined? This will be the case when validating a lot of IDL snippets, and will mean that we can't always know if the dictionary has required members or not.

[bzbarsky]

In general, one cannot properly validate "IDL snippets", because so much of IDL validation has to do with global state... Something somewhere really needs to be doing global validation of "all the IDL we have", ideally.

For example, you can't know whether a dictionary has partial members or not simply because there might be a partial dictionary hanging out somewhere not in your snippet that defines those members.

[foolip]

Yes, that is precisely the problem. Validating all IDL together in reffy-reports is doable, but what should webidl2.js do if given an incomplete definition? Is that itself a validation error, or to be silently ignored?

[bzbarsky]

Seems like it depends pretty strongly on the context and on what the consumer asking for validation really wants to know....

[foolip]

Yeah, configurable makes sense. @saschanaz does that seem OK?

[saschanaz]

The current general behavior is that any unknown identifier is silently ignored. I would expect being consistent here.

[foolip]

Hmm, if we wanted to validate that partial dictionaries don't have typos in the dictionary names, which has happened, would you consider that as an opt-in mode for the validator, or out of scope for this project?

The idlharness.js tests do add "original interface defined" subtests and similar for dictionaries, and I'd quite like to make that part of the validation step :)

[saschanaz]

Sounds good, but might depend on #413 because there are some cases that cannot access full context (e.g. the current version of ReSpec can't validate cross reference).

[marcoscaceres]

We might need to create a web service to access all the IDLs - then we can send down all external dependencies. We should chat about this on Sunday at the Hackathon.

[saschanaz]

I think we can defer reference validation as a separate feature, as it should also cover interfaces and probably return types, argument types, etc.

[saschanaz]

I'm working on this, but not sure what happens if an argument has a non-required dictionary type but not in final position.

dictionary BrilliantInit {
  any stuff;
};

[Exposed=Window]
interface X {
    constructor(BrilliantInit dict, DOMString type);
};

@bzbarsky Is this a supported situation?

[bzbarsky]

It's supported, yes, though it's not a great API and people probably should not create such an API.

What the spec says to do in this case is to throw if the constructor is called with fewer than 2 arguments, otherwise initialize BrilliantInit with the first argument. That will invoke https://heycam.github.io/webidl/#es-to-dictionary which will do whatever it does.

Similarly interesting is this case:

dictionary BrilliantInit {
  any stuff;
};

[Exposed=Window]
interface X {
    constructor(optional BrilliantInit dict, DOMString type);
};

For that one the spec currently says that the argument can be missing (if explicit undefined is passed as the first argument), but whatwg/webidl#76 tracks maybe changing that, including possibly disallowing such APIs. For what it's worth, as far as I know there are no such APIs in the web platform currently, and Gecko's code generator explicitly does not support this situation.