Skip to content
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

Replace inheritance_integrity by trait shenanigans #15567

Merged
merged 3 commits into from Feb 16, 2017
Merged

Conversation

@nox
Copy link
Member

nox commented Feb 15, 2017

This change is Reviewable

} else {
panic!("#[derive(DomObject)] should only be applied on proper structs")
};

let first_field_name = fields
.first()

This comment has been minimized.

@Ms2ger

Ms2ger Feb 15, 2017

Contributor

split_first?

This comment has been minimized.

@nox

nox Feb 15, 2017

Author Member

For some reason I thought this one panicked, will amend.

This comment has been minimized.

@nox

nox Feb 15, 2017

Author Member

Fixed.

@nox nox force-pushed the nox:plugin branch 2 times, most recently from 31fb6fe to 1d8b2b5 Feb 15, 2017
@SimonSapin
Copy link
Member

SimonSapin commented Feb 15, 2017

Is this covered by existing compile-fail tests?


Reviewed 1 of 1 files at r1, 3 of 4 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


components/domobject_derive/lib.rs, line 67 at r3 (raw file):

    items.append_all(field_types.iter().map(|ty| {
        quote! {
            impl #impl_generics ShouldNotImplDomObject for ((#params), #ty) #where_clause {}

This could use a code comment, in particular ((#params), #ty)


components/plugins/lints/inheritance_integrity.rs, line 30 at r3 (raw file):

        // Lints are run post expansion, so it's fine to use
        // #[_dom_struct_marker] here without also checking for #[dom_struct]
        if cx.tcx.has_attr(cx.tcx.hir.local_def_id(id), "_dom_struct_marker") {

Since this is removed, I think _dom_struct_marker can also be removed in plugins/lib.rs and plugins/jstraceable.rs.


components/plugins/lints/inheritance_integrity.rs, line 34 at r3 (raw file):

            let reflector_span = def.fields().iter().enumerate()
                                    .find(|&(ctr, f)| {
                                        if match_lang_ty(cx, &*f.ty, "reflector") {

I think this was the last use of match_lang_ty, which can therefore be removed.


Comments from Reviewable

For each derived DomObject impl, we also generate a dummy trait
ShouldNotImplDomObject that is implemented for all T: DomObject.
We then try to implement it for each field type except the first one.
If compilation succeed, this means that field type doesn't implement
DomObject itself otherwise it would break coherence rules.

error[E0119]: conflicting implementations of trait `dom::xmlhttprequest::_IMPL_DOMOBJECT_FOR_XMLHttpRequest::ShouldNotImplDomObject` for type `((), SomeFieldTypeThatShouldNotImplementDomObject)`:
   --> /Users/nox/src/servo/components/script/dom/xmlhttprequest.rs:120:1
    |
120 | #[dom_struct]
    | ^^^^^^^^^^^^^
    | |
    | first implementation here
    | conflicting implementation for `((), SomeFieldTypeThatShouldNotImplementDomObject)`
@nox nox force-pushed the nox:plugin branch from 1d8b2b5 to a6d59d8 Feb 15, 2017
@nox
Copy link
Member Author

nox commented Feb 15, 2017

@SimonSapin Addressed your remarks.

@nox
Copy link
Member Author

nox commented Feb 15, 2017

It's quite involved to make a test for it because you need to redefine the traits etc, given they are defined in script itself. It is also currently untested, so I am not sure it's worth making one as part of this PR.

@SimonSapin
Copy link
Member

SimonSapin commented Feb 16, 2017

@bors-servo r+


Reviewed 5 of 5 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@bors-servo
Copy link
Contributor

bors-servo commented Feb 16, 2017

📌 Commit 1464c11 has been approved by SimonSapin

@bors-servo
Copy link
Contributor

bors-servo commented Feb 16, 2017

Testing commit 1464c11 with merge 84a44a4...

bors-servo added a commit that referenced this pull request Feb 16, 2017
Replace inheritance_integrity by trait shenanigans

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15567)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 16, 2017

💔 Test failed - linux-rel-css

@nox
Copy link
Member Author

nox commented Feb 16, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Feb 16, 2017

Previous build results for android, arm32, arm64, linux-dev, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev are reusable. Rebuilding only linux-rel-css...

@bors-servo
Copy link
Contributor

bors-servo commented Feb 16, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: SimonSapin
Pushing 84a44a4 to master...

@bors-servo bors-servo merged commit 1464c11 into servo:master Feb 16, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.