928674 - Local build error with MSVC 2012 in AccessibleWrap.cpp(924) : error C2440: '=' : cannot convert from 'mozilla::a11y::RelationType' to 'int32_t' / This conversion requires an explicit cast (static_cast, C-style cast or function-style cast)

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Daniel Holbert [:dholbert]]

Build error when building mozilla-inbound on Windows (with Visual Studio 2012 Update 3):
{
/mozilla-central/accessible/src/windows/msaa/AccessibleWrap.cpp(924) : error C2440: '=' : cannot convert from 'mozilla::a11y::RelationType' to 'int32_t'
         This conversion requires an explicit cast (static_cast, C-style cast or function-style cast)
/mozilla-central/accessible/src/windows/msaa/AccessibleWrap.cpp(927) : error C2440: '=' : cannot convert from 'mozilla::a11y::RelationType' to 'int32_t'
         This conversion requires an explicit cast (static_cast, C-style cast or function-style cast)
/mozilla-central/accessible/src/windows/msaa/AccessibleWrap.cpp(930) : error C2440: '=' : cannot convert from 'mozilla::a11y::RelationType' to 'int32_t'
         This conversion requires an explicit cast (static_cast, C-style cast or function-style cast)
/mozilla-central/accessible/src/windows/msaa/AccessibleWrap.cpp(933) : error C2440: '=' : cannot convert from 'mozilla::a11y::RelationType' to 'int32_t'
         This conversion requires an explicit cast (static_cast, C-style cast or function-style cast)
/mozilla-central/accessible/src/windows/msaa/AccessibleWrap.cpp(936) : error C2440: '=' : cannot convert from 'mozilla::a11y::RelationType' to 'int32_t'
         This conversion requires an explicit cast (static_cast, C-style cast or function-style cast)
/mozilla-central/accessible/src/windows/msaa/AccessibleWrap.cpp(939) : error C2440: '=' : cannot convert from 'mozilla::a11y::RelationType' to 'int32_t'
         This conversion requires an explicit cast (static_cast, C-style cast or function-style cast)
/mozilla-central/accessible/src/windows/msaa/AccessibleWrap.cpp(942) : error C2440: '=' : cannot convert from 'mozilla::a11y::RelationType' to 'int32_t'
         This conversion requires an explicit cast (static_cast, C-style cast or function-style cast)
/mozilla-central/accessible/src/windows/msaa/AccessibleWrap.cpp(945) : error C2440: '=' : cannot convert from 'mozilla::a11y::RelationType' to 'int32_t'
         This conversion requires an explicit cast (static_cast, C-style cast or function-style cast)
/mozilla-central/accessible/src/windows/msaa/AccessibleWrap.cpp(948) : error C2440: '=' : cannot convert from 'mozilla::a11y::RelationType' to 'int32_t'
         This conversion requires an explicit cast (static_cast, C-style cast or function-style cast)
/mozilla-central/accessible/src/windows/msaa/AccessibleWrap.cpp(951) : error C2440: '=' : cannot convert from 'mozilla::a11y::RelationType' to 'int32_t'
         This conversion requires an explicit cast (static_cast, C-style cast or function-style cast)
/mozilla-central/accessible/src/windows/msaa/AccessibleWrap.cpp(954) : error C2440: '=' : cannot convert from 'mozilla::a11y::RelationType' to 'int32_t'
         This conversion requires an explicit cast (static_cast, C-style cast or function-style cast)
/mozilla-central/accessible/src/windows/msaa/AccessibleWrap.cpp(957) : error C2440: '=' : cannot convert from 'mozilla::a11y::RelationType' to 'int32_t'
         This conversion requires an explicit cast (static_cast, C-style cast or function-style cast)
/mozilla-central/accessible/src/windows/msaa/AccessibleWrap.cpp(960) : error C2440: '=' : cannot convert from 'mozilla::a11y::RelationType' to 'int32_t'
         This conversion requires an explicit cast (static_cast, C-style cast or function-style cast)
/mozilla-central/accessible/src/windows/msaa/AccessibleWrap.cpp(963) : error C2440: '=' : cannot convert from 'mozilla::a11y::RelationType' to 'int32_t'
         This conversion requires an explicit cast (static_cast, C-style cast or function-style cast)
/mozilla-central/accessible/src/windows/msaa/AccessibleWrap.cpp(966) : error C2440: '=' : cannot convert from 'mozilla::a11y::RelationType' to 'int32_t'
         This conversion requires an explicit cast (static_cast, C-style cast or function-style cast)
/mozilla-central/accessible/src/windows/msaa/AccessibleWrap.cpp(969) : error C2440: '=' : cannot convert from 'mozilla::a11y::RelationType' to 'int32_t'
         This conversion requires an explicit cast (static_cast, C-style cast or function-style cast)
/mozilla-central/accessible/src/windows/msaa/AccessibleWrap.cpp(972) : error C2440: '=' : cannot convert from 'mozilla::a11y::RelationType' to 'int32_t'
         This conversion requires an explicit cast (static_cast, C-style cast or function-style cast)
}

Looks like this class was recently added via MOZ_BEGIN_ENUM_CLASS(RelationType) in bug 748639:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e8f7b915ad46#l3.19
...and we're assigning values of this type into "xpRelation", which is what's triggering the error:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e8f7b915ad46#l21.11
...because xpRelation is declared as an integer:
http://hg.mozilla.org/integration/mozilla-inbound/annotate/e8f7b915ad46/accessible/src/windows/msaa/AccessibleWrap.cpp#l899

Presumably xpRelation's type should be changed.

Comment 1

[Daniel Holbert [:dholbert]]

It's absolutely correct that the compiler's erroring out on this, BTW - enum class types are supposed to be type-safe, so they can't be implicitly converted to int.

(I assume it only avoided blowing up on TBPL because (a) this is Windows-only code and (b) the compiler we use on Windows TBPL builders must be old enough to get the fallback (non-typesafe) path for MOZ_BEGIN_ENUM_CLASS -- i.e. it just translates to enum { ... }. or something like that.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1: just static_cast in the assignments

(In reply to Daniel Holbert [:dholbert] from comment #0)
> Presumably xpRelation's type should be changed.

Actually, it's not quite that simple; we initialize xpRelation as -1 (which is out of bounds for RelationType), so we can't change it unless we add a suitable "INVALID" value to its enum list that we could use in place of -1 here.

I don't know that it's worth adding such a value just for this one use case, though.  There are other ways we could restructure this code to not depend on -1, but that'd involve some refactoring which I don't really want to do.

So, here's the low-impact solution -- keep xpRelation an int32_t, and static_cast all of the enum values to its type when we do assignments.  It's ugly, but not really any uglier than what we already do elsewhere when dealing with RelationType values, really. (E.g. we already have a static_cast<RelationType)(xpRelation) later in this function.)

Comment 3

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0502bca5354d

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/0502bca5354d

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> It's absolutely correct that the compiler's erroring out on this, BTW - enum
> class types are supposed to be type-safe, so they can't be implicitly
> converted to int.

yeah, sorry I wasn't thinking when I reviewed that hunk :(

> (In reply to Daniel Holbert [:dholbert] from comment #0)
> > Presumably xpRelation's type should be changed.
> 
> Actually, it's not quite that simple; we initialize xpRelation as -1 (which
> is out of bounds for RelationType), so we can't change it unless we add a
> suitable "INVALID" value to its enum list that we could use in place of -1
> here.

actually another option that occurs to me know would be to make xpRelation's type MayBe<REelationType> but whatever someone can clean it up later

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> actually another option that occurs to me know would be to make xpRelation's
> type MayBe<REelationType> but whatever someone can clean it up later

Ah, good point.

I actually feel gross enough about having added these static_casts that I'd like to immediately replace them with your Maybe<> suggestion. Followup coming up to do that.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: followup: use Maybe<> instead

Comment 8

[Daniel Holbert [:dholbert]]

(pretend "followup" is in the commit message. I added it, but posted the patch before qreffing, and now my computer is being difficult so I'm not bothering to repost)

Comment 9

[Trevor Saunders (:tbsaunde)]

Comment on attachment 820017 [details] [diff] [review]
followup: use Maybe<> instead

I wish MayBe allowed you to just assign a T to it, but oh well

Comment 10

[Daniel Holbert [:dholbert]]

Landed followup: https://hg.mozilla.org/integration/mozilla-inbound/rev/07dec571ca95

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/07dec571ca95