New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Breaking change when switching on Xml.nodeType (3.2.0-rc2) #4084

Closed
jasononeil opened this Issue Mar 26, 2015 · 7 comments

Comments

Projects
None yet
2 participants
@jasononeil
Contributor

jasononeil commented Mar 26, 2015

Previously this code worked (Haxe 3.1.3):

    switch x.nodeType {
        case Xml.ProcessingInstruction:
        case Xml.DocType:
        case Xml.Document:
        case Xml.CData:
        case Xml.PCData:
        case Xml.Element:
        case Xml.Comment:
    }

Now you must use this code (Haxe 3.2.0-rc2):

    switch x.nodeType {
        case ProcessingInstruction:
        case DocType:
        case Document:
        case CData:
        case PCData:
        case Element:
        case Comment:
    }

I'm not stressed if this is a breaking change as part of the otherwise great Xml fixes. It would be good to include it in this list though: https://github.com/HaxeFoundation/haxe/wiki/Breaking-changes-in-Haxe-3.2.0

See http://try.haxe.org/#9a95b

@jasononeil

This comment has been minimized.

Show comment
Hide comment
@jasononeil

jasononeil Mar 26, 2015

Contributor

I forgot to include the error message:

Unmatched patterns: Document | ProcessingInstruction | DocType | Comment | CData | PCData | Element

Contributor

jasononeil commented Mar 26, 2015

I forgot to include the error message:

Unmatched patterns: Document | ProcessingInstruction | DocType | Comment | CData | PCData | Element

@Simn

This comment has been minimized.

Show comment
Hide comment
@Simn

Simn Mar 26, 2015

Member

Finally a real issue. \o/ Let's hope I break something with the fix.

Member

Simn commented Mar 26, 2015

Finally a real issue. \o/ Let's hope I break something with the fix.

@Simn Simn self-assigned this Mar 26, 2015

@Simn Simn added this to the 3.2 milestone Mar 26, 2015

@Simn Simn closed this in c189af4 Mar 26, 2015

@jasononeil

This comment has been minimized.

Show comment
Hide comment
@jasononeil

jasononeil Mar 26, 2015

Contributor

I've got a few more, once I can reproduce ;)

On Thu, Mar 26, 2015 at 6:43 PM, Simon Krajewski notifications@github.com
wrote:

Closed #4084 #4084 via
c189af4
c189af4
.


Reply to this email directly or view it on GitHub
#4084 (comment).

Contributor

jasononeil commented Mar 26, 2015

I've got a few more, once I can reproduce ;)

On Thu, Mar 26, 2015 at 6:43 PM, Simon Krajewski notifications@github.com
wrote:

Closed #4084 #4084 via
c189af4
c189af4
.


Reply to this email directly or view it on GitHub
#4084 (comment).

@Simn

This comment has been minimized.

Show comment
Hide comment
@Simn

Simn Nov 19, 2015

Member

It turns out this was actually failing for a good reason, if with the wrong error message. All these static fields on Xml are not inline/read-only, so inlining their expression is not sound. In fact, this is also broken for the plain identifier case (case Element:) which inlines the expression value.

Member

Simn commented Nov 19, 2015

It turns out this was actually failing for a good reason, if with the wrong error message. All these static fields on Xml are not inline/read-only, so inlining their expression is not sound. In fact, this is also broken for the plain identifier case (case Element:) which inlines the expression value.

@Simn Simn reopened this Nov 19, 2015

@Simn

This comment has been minimized.

Show comment
Hide comment
@Simn

Simn Nov 19, 2015

Member

Actually case Element: is sound: Here top-down inference identifies it as a constructor of XmlType, which is not the same as Xml.Element. When I fixed this issue I assumed that the two were equivalent when in fact they are not.

Long story short I think case Xml.Element: should indeed fail because it doesn't/shouldn't resolve to a constant value.

Member

Simn commented Nov 19, 2015

Actually case Element: is sound: Here top-down inference identifies it as a constructor of XmlType, which is not the same as Xml.Element. When I fixed this issue I assumed that the two were equivalent when in fact they are not.

Long story short I think case Xml.Element: should indeed fail because it doesn't/shouldn't resolve to a constant value.

@Simn Simn closed this in 62bcf79 Nov 19, 2015

@Simn

This comment has been minimized.

Show comment
Hide comment
@Simn

Simn Nov 19, 2015

Member

Stupid revert commits closing issues. Anyway, this is not an issue an working as expected, so it can remain closed.

Member

Simn commented Nov 19, 2015

Stupid revert commits closing issues. Anyway, this is not an issue an working as expected, so it can remain closed.

@jasononeil

This comment has been minimized.

Show comment
Hide comment
@jasononeil

jasononeil Nov 20, 2015

Contributor

Let's hope I break something with the fix.

You got what you hoped for!

Thanks for the explanation, it makes sense. On a different note, I'm really surprised that those Xml static fields are not inlined / constant. Changing them would cause all kinds of Xml chaos.

Contributor

jasononeil commented Nov 20, 2015

Let's hope I break something with the fix.

You got what you hoped for!

Thanks for the explanation, it makes sense. On a different note, I'm really surprised that those Xml static fields are not inlined / constant. Changing them would cause all kinds of Xml chaos.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment