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

Webidl lint #23200

Merged
merged 12 commits into from Apr 23, 2019

Conversation

Projects
None yet
6 participants
@krk
Copy link
Contributor

commented Apr 14, 2019

Parse webidl files and lint for inheritance correctness.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #20461

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Apr 14, 2019

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @SimonSapin (or someone else) soon.

@highfive

This comment has been minimized.

Copy link

commented Apr 14, 2019

Heads up! This PR modifies the following files:

@highfive

This comment has been minimized.

Copy link

commented Apr 14, 2019

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
}
}

fn get_typ_name(typ: String) -> String {

This comment has been minimized.

Copy link
@KiChjang

KiChjang Apr 14, 2019

Member
  • Since type is a keyword, by convention we usually shorten it to ty.
  • Is get_typ_name a typo?
  • Can this function receive a &str instead of a String?

This comment has been minimized.

Copy link
@krk

krk Apr 16, 2019

Author Contributor

It is not a typo, will change to ty and get_ty_name.

dir.push("components/script/dom/webidls/");
dir.push(format!("{}.webidl", struct_name));

return Ok(dir);

This comment has been minimized.

Copy link
@KiChjang

KiChjang Apr 14, 2019

Member

nit: No need for explicit return here.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

☔️ The latest upstream changes (presumably #23197) made this pull request unmergeable. Please resolve the merge conflicts.

@krk krk force-pushed the krk:webidl-lint branch from 219a97e to 59ccf4a Apr 16, 2019


impl fmt::Display for WebIdlError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {

This comment has been minimized.

Copy link
@KiChjang

KiChjang Apr 16, 2019

Member

It is sort of weird seeing a match here being used solely for destructuring. You can actually instead do the following:

let WebIdlError { name, rust_parent, webidl_parent } = self;

or simply just use self.name, self.rust_parent and self.webidl_parent without destructuring.

rust_parent,
webidl_parent,
} => {
return write!(f, "webidl-rust inheritance mismatch, rust: {:?}, rust parent: {:?}, webidl parent: {:?}",

This comment has been minimized.

Copy link
@KiChjang

KiChjang Apr 16, 2019

Member

Following on my review comment above, I would actually write this in a one-liner:

// Note the lack of the return keyword
write!(f, "webidl-rust inheritance mismatch, rust: {:?}, rust parent, {:?}, WebIDL parent: {:?}", self.name, self.rust_parent, self.webidl_parent)

This comment has been minimized.

Copy link
@krk

krk Apr 16, 2019

Author Contributor

This one does not compile as WebIdlError contains a member ParentMismatch.

This comment has been minimized.

Copy link
@krk

krk Apr 16, 2019

Author Contributor

I replaced WebIdlError enum with the new ParentMismatchError.


let ty: String;
let mut parent_name: Option<&str> = None;
for ref field in def.fields() {

This comment has been minimized.

Copy link
@KiChjang

KiChjang Apr 16, 2019

Member
let parent_name = def.fields().next().map(|field| {
    let def_id = cx.tcx.hir().local_def_id_from_hir_id(field.hir_id);
    let ty = cx.tcx.type_of(def_id);
    get_ty_name(ty).to_string()
});
@KiChjang

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

error: webidl-rust inheritance mismatch, rust: "TestBindingProxy", rust parent: "Reflector", webidl parent: "TestBinding"
  --> components/script/dom/testbindingproxy.rs:13:12
   |
13 | pub struct TestBindingProxy {
   |            ^^^^^^^^^^^^^^^^
   |
   = note: #[deny(webidl_inherit_correct)] on by default
error: No such file or directory (os error 2)
  --> components/script/dom/windowproxy.rs:59:12
   |
59 | pub struct WindowProxy {
   |            ^^^^^^^^^^^
error: aborting due to 2 previous errors
error: Could not compile `script`.
To learn more, run the command again with --verbose.
The command "./mach test-unit" exited with 101.
@KiChjang
Copy link
Member

left a comment

Wait a minute, this doesn't look like the correct change... when there's a [NoInterfaceObject] annotation, it should mean that the corresponding Rust struct doesn't inherit from anything, so we should skip the inheritance analysis instead.

@krk

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

The lint passes for WindowProxy because we evaluate "doesn't inherit from anything" as "it must inherit from the Reflector", and it inherits it. Should we skip checking the Reflector for [NoInterfaceObject]?

@KiChjang

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

I think we should special case WindowProxy because it shares the same Reflector with a Window, and hence why it appears in window.webidl instead of windowproxy.webidl. We should also check for its first field to ensure that it's a reflector.

@KiChjang

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

Oh, I didn't know that you were already asking for solutions in the original issue. Please follow what @jdm has advised and ignore my comments.

@krk krk force-pushed the krk:webidl-lint branch from b5a660e to b86766b Apr 19, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2019

☔️ The latest upstream changes (presumably #23234) made this pull request unmergeable. Please resolve the merge conflicts.

@krk krk force-pushed the krk:webidl-lint branch from b86766b to fea2ad0 Apr 20, 2019

@krk

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

@jdm, @KiChjang; now that the changes #20461 (comment) and #20461 (comment) are committed, do you think that this PR is mergeable?

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

@bors-servo r+
Nice work! Thanks for tackling this issue!

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

📌 Commit fea2ad0 has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

⌛️ Testing commit fea2ad0 with merge 4c8d29f...

bors-servo added a commit that referenced this pull request Apr 23, 2019

Auto merge of #23200 - krk:webidl-lint, r=jdm
Webidl lint

Parse webidl files and lint for inheritance correctness.

---

<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #20461

<!-- 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/23200)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@bors-servo bors-servo merged commit fea2ad0 into servo:master Apr 23, 2019

4 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@krk

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

@KiChjang thanks for the comprehensive review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.