1224918 - find a better solution for casting away enum class safety for enumerated css property values

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

In bug 1223653 patch 2, I have to add a few casts to cast away the safety we get from enum classes, as part of the process of introducing the use of enum classes for enumerated values of CSS properties.

However, I'd like to find a better solution for this.  bug 1224464 is the first piece of that, but I'm not quite sure how to continue.  There's some discussion in https://groups.google.com/d/topic/mozilla.dev.tech.layout/eYJkMF84rsE/discussion .

Comment 1

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

xidorn has proposals for how to do this in bug 1223653.

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #1)
> xidorn has proposals for how to do this in bug 1223653.

Actually, those comments were, I think, about a different problem, which is how to avoid changing the value passed for the ignored parameters to SetDiscrete, rather than avoiding the need for the casts inside of SetDiscrete.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

That also avoids casts inside of SetDiscrete, no?

Reading the patch in bug 1223653, it seems to me the hardest part of this bug would be fixing the casts in nsCSSProps.cpp. Making KTableEntry a template doesn't work perfectly, because we use a sentinel entry "{ eCSSKeyword_Unknown, -1 }" to mark the end of tables, and if we keep doing so, we would have to make all enums base on signed integer, and cast the -1 to corresponding enum classes.

I feel like it would be better to switch to checking the count instead of the sentinel entry. But the count number should be computed automatically. It seems to be challenging to keep the syntax simple while doing so, especially in compile time.

I do not have particually good idea here. If we have the power of macros in Rust, it would be a fairly simple task :)

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

There would also be an issue for kKeywordTableTable and any use of it.

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #3)
> That also avoids casts inside of SetDiscrete, no?

I don't see how.  The casts I had to add to SetDiscrete were to the results of aValue.GetIntValue().

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #5)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #3)
> > That also avoids casts inside of SetDiscrete, no?
> 
> I don't see how.  The casts I had to add to SetDiscrete were to the results
> of aValue.GetIntValue().

Ah, you meant that. Right, that doesn't solve those casts. I somehow ignored that...

For the eCSSUnit_Integer case, I think we could have another helper template function, which asserts when the target type is not convertible from an integer. (Then we no longer need the SETDSC_INTEGER flag.)

For the eCSSUnit_Enumerated case, I don't have particular good idea. There's even no automatic way in runtime to ensure that the value is in the proper range, because there's no way in C++ to detect the max/min value of a enum.

One potential solution is to have a helper template function which asserts by default, and we manually instantiate it for each enum class we use, and optionally add runtime check there. (We probably can define a macro to reduce the code need to write for each individual enum class.) The only benefit AFAICS is that we can get rid of the SETDSC_ENUMERATED flag (and thus remove a runtime check which is actually never used) while still keep things safe.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: proposal patch for SetDiscrete

This is what I'm currently thinking for SetDiscrete.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: proposal patch for SetDiscrete

Updated a bit compared with the last patch.

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8723420 [details] [diff] [review]
proposal patch for SetDiscrete

I guess this is an improvement on what we currently have, although I'd prefer to get compilation errors to indicate failure rather than assertions.

(I wonder if there's a way to get that by making SetDiscrete a template over the flags rather than passing the flags as a parameter?  Although I wonder if that reduces or increases code size...)

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

I don't think it is possible to use compilation error for that, since aValue.GetUnit() is only known at runtime, which means there would be some form of assertions anyway.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1224918 part 1 - Make SetDiscrete more type-safe and easy to use with enum classes.

Review commit: https://reviewboard.mozilla.org/r/56810/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56810/

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1224918 part 2 - Remove SETDCT_{AUTO,NONE,NORMAL,SYSTEM_FONT} and use Unused to indicate unsupported units.

Review commit: https://reviewboard.mozilla.org/r/56812/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56812/

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8758561 [details]
Bug 1224918 part 1 - Make SetDiscrete more type-safe and easy to use with enum classes.

https://reviewboard.mozilla.org/r/56810/#review54158

Maybe it's worth a FIXME comment that it would be nice to make the things that currently MOZ_ASSERT_UNREACHABLE into compilation errors if we can figure out how?

Comment 14

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8758562 [details]
Bug 1224918 part 2 - Remove SETDCT_{AUTO,NONE,NORMAL,SYSTEM_FONT} and use Unused to indicate unsupported units.

https://reviewboard.mozilla.org/r/56812/#review54160

::: layout/style/nsRuleNode.cpp:1342
(Diff revision 1)
>  struct SetDiscreteHelper : Conditional<IsEnum<FieldT>::value,
>                                         SetEnumDiscreteHelper,
>                                         SetIntegerDiscreteHelper<FieldT>>::Type
>  {
>  };
>  

The commit message should say "unsupported units" rather than "unsupport units".

::: layout/style/nsRuleNode.cpp:1354
(Diff revision 1)
> -          typename T1, typename T2, typename T3, typename T4, typename T5>
> +          typename AutoT = unused_t, typename NoneT = unused_t,
> +          typename NormalT = unused_t, typename SysFontT = unused_t>

Are these "= unused_t" here needed or helpful, given that you also have "= Unused" on the values?

::: layout/style/nsRuleNode.cpp
(Diff revision 1)
> -              SETDSC_NONE | SETDSC_AUTO | SETDSC_UNSET_INHERIT,
>                parentText->mTextSizeAdjust,
> -              NS_STYLE_TEXT_SIZE_ADJUST_AUTO, // initial value
> -              NS_STYLE_TEXT_SIZE_ADJUST_AUTO, // auto value
> -              NS_STYLE_TEXT_SIZE_ADJUST_NONE, // none value
> +              /* initial */ NS_STYLE_TEXT_SIZE_ADJUST_AUTO,
> +              /* auto */ NS_STYLE_TEXT_SIZE_ADJUST_AUTO,
> +              /* none */ NS_STYLE_TEXT_SIZE_ADJUST_NONE);
> -              0, 0);

I'm not crazy about having varying numbers of these parameters; I'd somewhat prefer to have either none of the four, or all 4; I think that's easier to read.

So I'd prefer to have an "Unused, Unused" at the end here.

::: layout/style/nsRuleNode.cpp:5745
(Diff revision 1)
>  
>    // contain: none, enum, inherit, initial
>    SetDiscrete(*aRuleData->ValueForContain(), display->mContain, conditions,
> -              SETDSC_ENUMERATED | SETDSC_NONE | SETDSC_UNSET_INITIAL,
> +              SETDSC_ENUMERATED | SETDSC_UNSET_INITIAL,
>                parentDisplay->mContain,
> -              NS_STYLE_CONTAIN_NONE, 0, NS_STYLE_CONTAIN_NONE, 0, 0);
> +              NS_STYLE_CONTAIN_NONE, Unused, NS_STYLE_CONTAIN_NONE);

and 2 Unuseds here as well

::: layout/style/nsRuleNode.cpp:6008
(Diff revision 1)
> -              SETDSC_ENUMERATED | SETDSC_AUTO | SETDSC_NONE |
> +              SETDSC_ENUMERATED | SETDSC_UNSET_INITIAL,
> -                SETDSC_UNSET_INITIAL,
>                parentDisplay->mTouchAction,
> -              NS_STYLE_TOUCH_ACTION_AUTO,
> -              NS_STYLE_TOUCH_ACTION_AUTO,
> -              NS_STYLE_TOUCH_ACTION_NONE, 0, 0);
> +              /* initial */ NS_STYLE_TOUCH_ACTION_AUTO,
> +              /* auto */ NS_STYLE_TOUCH_ACTION_AUTO,
> +              /* none */ NS_STYLE_TOUCH_ACTION_NONE);

same here

<p>... in fact, given my comments above, I think I'd prefer to make it impossible to have an intermediate number of those parameters.</p>
<p>I think that should be easy to do by using a function overload (a variant using 4 fewer parameters, calling the other variant with 4 extra Unuseds) instead of default parameters.</p>
<p></p>
<p></p>
<p>Also, after you land this, please double-check that things are ok a few days after you land, and nobody's landed patches that disagree with it.</p>
<p>r=dbaron with that</p>

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

It's unfortunate that it won't lead to any error when someone uses the old form and pass 0s in. Probably we should rename the function a bit so that we can catch any new use of that function?

Comment 16

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I don't have any great naming ideas, though.  Maybe SetUntagged (since it's setting a value that's not part of a tagged union, like SetCoord does)?  Or maybe just SetValue?

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

I guess SetValue works. Thanks for the suggestions.

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1224918 part 0 - Rename SetDiscrete to SetValue so that we can catch any new use of the old pattern with a compilation error.

Review commit: https://reviewboard.mozilla.org/r/57414/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57414/

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8758561 [details]
Bug 1224918 part 1 - Make SetDiscrete more type-safe and easy to use with enum classes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56810/diff/1-2/

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8758562 [details]
Bug 1224918 part 2 - Remove SETDCT_{AUTO,NONE,NORMAL,SYSTEM_FONT} and use Unused to indicate unsupported units.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56812/diff/1-2/

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

dbaron, could you review the new rename patch (part 0)? I guess you may also want to have a look at the updated part 1, which is changed quite a bit.

Comment 22

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8759436 [details]
Bug 1224918 part 0 - Rename SetDiscrete to SetValue so that we can catch any new use of the old pattern with a compilation error.

https://reviewboard.mozilla.org/r/57414/#review54222

Seems like it would have been easier to write this on top of the other two, but I guess it's done now.

Comment 23

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

... and moving things into SetValueHelper seems fine.

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=732bd10e71d9

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8759436 [details]
Bug 1224918 part 0 - Rename SetDiscrete to SetValue so that we can catch any new use of the old pattern with a compilation error.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57414/diff/1-2/

Comment 26

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8758561 [details]
Bug 1224918 part 1 - Make SetDiscrete more type-safe and easy to use with enum classes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56810/diff/2-3/

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8758562 [details]
Bug 1224918 part 2 - Remove SETDCT_{AUTO,NONE,NORMAL,SYSTEM_FONT} and use Unused to indicate unsupported units.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56812/diff/2-3/

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e8ef068956b
part 0 - Rename SetDiscrete to SetValue so that we can catch any new use of the old pattern with a compilation error. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e6d4982db0b
part 1 - Make SetDiscrete more type-safe and easy to use with enum classes. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f8b79672ffe
part 2 - Remove SETDCT_{AUTO,NONE,NORMAL,SYSTEM_FONT} and use Unused to indicate unsupported units. r=dbaron

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8e8ef068956b
https://hg.mozilla.org/mozilla-central/rev/4e6d4982db0b
https://hg.mozilla.org/mozilla-central/rev/1f8b79672ffe