New issue
Advanced search Search tips

Issue 894469 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 29
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

WebIDL: Two union types with constituent types that only differ in attributes need to be distinct.

Project Member Reported by vogelheim@chromium.org, Oct 11

Issue description

In WebIDL, I need to specify two union types that only differ in the attributes that apply to the string.

typedef (DOMString or TrustedScript) ScriptString;
typedef ([TreatNullAs=NullString] DOMString or TrustedScript) ScriptStringWithExtras;

The IDL compiler generates the same C++ and file names for either: 
  class StringOrTrustedScript;
  string_or_trusted_script.{h,cc}

This means that they cannot co-exist.

I require the TreatNullAs attribute to remain compatible with existing APIs, so I also cannot easily change my design to avoid this situation.


--------------


I presume the intent is that structurally identical types should be identical (e.g., (A or B) == (B or A)). If so, this would make a lot of sense, except that the attribute makes those two string types actually slightly different.

Context 1: crrev.com/c/1178046

Context 2: Union types w/ attributes do seem to occur, but are clearly not very common:

https://cs.chromium.org/search/?q=file:%5C.idl$+%5C(%5C%5B.*%5Cbor%5Cb&sq=package:chromium&type=cs

-------------------------------

Chrome Version: M70
OS: All, presumably.

What steps will reproduce the problem?
Have an .idl with both defintions given above, and see what they produce.

What is the expected result?
There needs to be a way to distinguish between the two helper classes. (I'm not sure about the file name. Both variants being generated into the same file would be ok, too.)

What happens instead?
The have the same name.


 
Status: Assigned (was: Untriaged)
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 16

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0f379f76d8fabe311fd16748e9571ff9c2aa5baa

commit 0f379f76d8fabe311fd16748e9571ff9c2aa5baa
Author: Hitoshi Yoshida <peria@chromium.org>
Date: Tue Oct 16 12:52:39 2018

IDL parser: Make IDL parser accept type annotation in union

Before this CL, IDL parser didn't accept types with
extended atttributes as union members.
This CL makes it possible.

For example,
  ([TreatNullAs=EmptyString] DOMString or [Clamp] long)
was an error, but it became acceptable.


Bug:  894469 
Change-Id: I2980256f5090f4465bef1b2a8665b0a9a9643b76
Reviewed-on: https://chromium-review.googlesource.com/c/1278614
Commit-Queue: Hitoshi Yoshida <peria@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599955}
[modify] https://crrev.com/0f379f76d8fabe311fd16748e9571ff9c2aa5baa/tools/idl_parser/idl_parser.py
[modify] https://crrev.com/0f379f76d8fabe311fd16748e9571ff9c2aa5baa/tools/idl_parser/test_parser/typedef_web.idl

Project Member

Comment 3 by bugdroid1@chromium.org, Oct 26

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/011bdf38cb1eb71c73e0f35da58ad115760f7c71

commit 011bdf38cb1eb71c73e0f35da58ad115760f7c71
Author: Hitoshi Yoshida <peria@chromium.org>
Date: Fri Oct 26 09:13:27 2018

IDL compiler: Create IdlAnnotatedType class

To work with IDL types with extended attributes (aka. annotated types),
this CL defines IdlAnnotatedType class.
https://heycam.github.io/webidl/#idl-annotated-types

Bug:  894469 
Change-Id: I2b59a391ada7657057f79685bad89f56a32a9349
Reviewed-on: https://chromium-review.googlesource.com/c/1296929
Commit-Queue: Hitoshi Yoshida <peria@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603038}
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/core/v8/idl_types.h
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/core/v8/native_value_traits_impl.h
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/scripts/idl_definitions.py
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/scripts/idl_types.py
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/scripts/v8_types.py
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/tests/results/core/long_or_boolean.cc
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/tests/results/core/long_or_test_dictionary.cc
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/tests/results/core/test_enum_or_test_enum_or_null_sequence.cc
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/tests/results/core/test_interface_or_long.cc
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/tests/results/core/unsigned_long_long_or_boolean_or_test_callback_interface.cc
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/tests/results/core/v8_test_dictionary.cc
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/tests/results/core/v8_test_dictionary_derived.cc
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/tests/results/core/v8_test_integer_indexed.cc
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/tests/results/core/v8_test_integer_indexed_global.cc
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/tests/results/core/v8_test_integer_indexed_primary_global.cc
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/tests/results/core/v8_test_interface.cc
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/tests/results/core/v8_test_interface_2.cc
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/tests/results/core/v8_test_interface_check_security.cc
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/tests/results/core/v8_test_interface_constructor.cc
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/tests/results/core/v8_test_interface_constructor_2.cc
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/tests/results/core/v8_test_interface_named_constructor.cc
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/tests/results/core/v8_test_interface_origin_trial_enabled.cc
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/tests/results/core/v8_test_object.cc
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/tests/results/core/v8_test_typedefs.cc
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/tests/results/modules/v8_test_interface_5.cc
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/tests/results/modules/v8_test_interface_partial.cc
[modify] https://crrev.com/011bdf38cb1eb71c73e0f35da58ad115760f7c71/third_party/blink/renderer/bindings/tests/results/modules/v8_test_sub_object.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 26

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/72f86a1d43c59f22605e3d53893ee998879a69b8

commit 72f86a1d43c59f22605e3d53893ee998879a69b8
Author: Hitoshi Yoshida <peria@chromium.org>
Date: Fri Oct 26 10:09:52 2018

IDL compiler: Use inner type names for IDLUnion member accessors

Generally we use type names of each member as accessor names
in generated union implementations.
But for annotated types, the difference from its inner type is
the conversion from V8 values to native values. i.e. NativeValueTraits.
It means that their C++ accessors for each member work as same with
their inner types' ones.
And it is annoying to use annotated type's names for other objectives
like IsLongClamp() and SetStringTreatNullAsEmptyString("foo").

So this CL makes the generator to use inner type names in
IDLUnion member accessors.
It means we do not have to consider if a member has extended
attributes or not, when we access the members.


Bug:  894469 
Change-Id: I80a6ea2366405a7ae344d740e1611cea20b88d9e
Reviewed-on: https://chromium-review.googlesource.com/c/1280382
Commit-Queue: Hitoshi Yoshida <peria@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603047}
[modify] https://crrev.com/72f86a1d43c59f22605e3d53893ee998879a69b8/third_party/blink/renderer/bindings/scripts/v8_union.py
[modify] https://crrev.com/72f86a1d43c59f22605e3d53893ee998879a69b8/third_party/blink/renderer/bindings/tests/idls/core/test_dictionary.idl
[add] https://crrev.com/72f86a1d43c59f22605e3d53893ee998879a69b8/third_party/blink/renderer/bindings/tests/results/core/string_treat_null_as_empty_string_or_long.cc
[add] https://crrev.com/72f86a1d43c59f22605e3d53893ee998879a69b8/third_party/blink/renderer/bindings/tests/results/core/string_treat_null_as_empty_string_or_long.h
[modify] https://crrev.com/72f86a1d43c59f22605e3d53893ee998879a69b8/third_party/blink/renderer/bindings/tests/results/core/test_dictionary.cc
[modify] https://crrev.com/72f86a1d43c59f22605e3d53893ee998879a69b8/third_party/blink/renderer/bindings/tests/results/core/test_dictionary.h
[modify] https://crrev.com/72f86a1d43c59f22605e3d53893ee998879a69b8/third_party/blink/renderer/bindings/tests/results/core/v8_test_dictionary.cc

Status: Fixed (was: Assigned)
#3 should fix the issue, although #4 introduced another bug.
It'll be tracked in another issue.

Sign in to add a comment