Add eslint checks to CI #85285

GuillaumeGomez · 2021-05-14T11:59:19Z

It also allowed me to fix some potential issues that went unnoticed. Having this process automated will hopefully prevent us to add more errors. :)

cc @Mark-Simulacrum (for the add in the CI).
r? @jsha

rust-highfive · 2021-05-14T11:59:21Z

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

jyn514

Is there a way for this to print the command for replicating the error when it fails?

jyn514 · 2021-05-14T12:45:27Z

src/ci/docker/host-x86_64/mingw-check/Dockerfile

@@ -37,4 +38,5 @@ ENV SCRIPT python3 ../x.py --stage 2 test src/tools/expand-yaml-anchors && \
           python3 ../x.py doc --stage 0 library/std && \
           /scripts/validate-toolstate.sh && \
           # Runs checks to ensure that there are no ES5 issues in our JS code.
-           es-check es5 ../src/librustdoc/html/static/*.js
+           es-check es5 ../src/librustdoc/html/static/*.js && \
+	   eslint ../src/librustdoc/html/static/*.js


Wouldn't it be better to make this part of x.py? That way it's easier for people to run locally, this is really hard to find.

Well, installing eslint isn't "easy" (requires to have npm then to run npm install eslint and if you're lucky, everything has been installed correctly) and I have no idea how all of that can work on windows. Considering that it fails pretty quickly and everyone has access to the log, I think it's enough.

Also I juste realized that I added a tab instead of whitespaces...

jsha

This is great! Nice work.

src/librustdoc/html/static/main.js

src/librustdoc/html/static/search.js

jsha · 2021-05-14T14:59:36Z

src/librustdoc/html/static/search.js

@@ -254,7 +257,7 @@ window.initSearch = function(rawSearchIndex) {
            });

            for (i = 0, len = results.length; i < len; ++i) {
-                var result = results[i];


I think this code was correct; result is only needed inside the scope of this for loop. The problem is up at line 178, where var result is declared with a large scope than it needs. Instead that one should be moved inside the for loop like this one. Arguably the same is true of i and len: narrowness of scope decrees the loops should be something like for (var i = 0, len = results.length. Though also the fact we have two loops that use the same vars suggests the function is a bit too big.

So, actually, it's an issue because when when you create a var inside a loop, its scope is still the whole function. So no, we need to keep this change unfortunately.

Oh dang, I was wondering if I was misremembering JS's scoping rules. That sounds right. How about using a different variable name to indicate that the lower for loop doesn't depend on output from the upper for loop?

They're actually both at the same level. result is just used in a loop later on. I really miss "normal" scopes when doing JS...

Ah, by "lower" and "upper" I meant lower in the file and higher in the file, not in different scopes. But this is not too important.

I'll keep things as is then.

src/librustdoc/html/static/search.js

src/librustdoc/html/static/source-script.js

src/librustdoc/html/static/storage.js

jsha · 2021-05-14T15:05:12Z

src/librustdoc/html/static/storage.js

@@ -84,7 +84,7 @@ function onEachLazy(lazyArray, func, reversed) {
 }

 // eslint-disable-next-line no-unused-vars
-function hasOwnProperty(obj, property) {
+function hasOwnPropertyRustdoc(obj, property) {


What was the lint finding that prompted the rename?

It was complaining about the fact that hasOwnProperty shouldn't be overloaded.

This is a polyfill, right? Is it still needed? It looks like it has very wide support: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/hasOwnProperty.

True, since we don't support IE anymore, should be fine to get rid of it now.

Like said below, eslint complains about it...

GuillaumeGomez · 2021-05-14T17:53:44Z

When I removed the hasOwnPropertyRustdoc, eslint is complaining about it:

/home/imperio/rust/rust/src/librustdoc/html/static/search.js
   176:29  error  Do not access Object.prototype method 'hasOwnProperty' from target object  no-prototype-builtins
  1248:67  error  Do not access Object.prototype method 'hasOwnProperty' from target object  no-prototype-builtins
  1299:33  error  Do not access Object.prototype method 'hasOwnProperty' from target object  no-prototype-builtins
  1389:34  error  Do not access Object.prototype method 'hasOwnProperty' from target object  no-prototype-builtins
  1393:41  error  Do not access Object.prototype method 'hasOwnProperty' from target object  no-prototype-builtins

jsha · 2021-05-14T20:13:31Z

Ah, got it. It looks like hasOwnProperty (or hasOwnPropertyRustdoc) is not actually a polyfill but a shortcut. According to https://eslint.org/docs/rules/no-prototype-builtins you should call hasOwnProperty like so:

Object.prototype.hasOwnProperty.call(foo, "bar");

But that's pretty awkward! I'm guessing that's why we have a function that wraps it. That part looks good to me now.

GuillaumeGomez · 2021-05-14T20:15:36Z

👍

src/librustdoc/html/static/source-script.js

jsha · 2021-05-14T20:30:50Z

@bors r+

bors · 2021-05-14T20:30:51Z

📌 Commit dcf85c3233b9e28718d7ee243df26306b3dd2c9f has been approved by jsha

jyn514 · 2021-05-14T21:27:22Z

@bors r-

Please land the eslint fixes separately from enforcing them in CI, I'd like @Mark-Simulacrum (or I guess me, but it will take a while) to take a look first. The current setup is very unusual.

…=jsha Fix eslint errors I cherry-picked the two non-CI commits from rust-lang#85285. r? `@jsha`

GuillaumeGomez · 2021-05-15T11:17:52Z

Done in #85323. Let's wait for it to get merged and then I'll remove the non-CI catch.

…=jsha Fix eslint errors I cherry-picked the two non-CI commits from rust-lang#85285. r? ``@jsha``

…=jsha Fix eslint errors I cherry-picked the two non-CI commits from rust-lang#85285. r? ```@jsha```

GuillaumeGomez · 2021-05-15T15:19:15Z

Rebased, now let's wait for @Mark-Simulacrum. :)

GuillaumeGomez · 2021-05-28T14:44:57Z

r? @Mark-Simulacrum

Mark-Simulacrum · 2021-05-29T20:47:49Z

@bors r=jsha,Mark-Simulacrum

I agree that the checks here aren't really standard, and that we may want to explore alternatives (including x.py integration, such as we did with the rustdoc-gui tests). Given that these are stylistic checks though I'm OK leaving them here for now, especially given that this is just adding an extra layer atop the existing es5 check.

bors · 2021-05-29T20:47:51Z

📌 Commit c9b4c5afeba6a64b19f2ae3e3d9f943ebd869149 has been approved by jsha,Mark-Simulacrum

bors · 2021-05-30T17:23:18Z

⌛ Testing commit c9b4c5afeba6a64b19f2ae3e3d9f943ebd869149 with merge b143d0aa0b95e7bbc8f48e1c3749cd90c24ea3a4...

bors · 2021-05-30T17:39:40Z

💔 Test failed - checks-actions

GuillaumeGomez · 2021-05-30T19:03:31Z

Fixed missing eslint for newly added JS file and rebased (not in that order).

@bors: r=jsha,Mark-Simulacrum

bors · 2021-05-30T19:03:32Z

📌 Commit 558b073 has been approved by jsha,Mark-Simulacrum

…laumeGomez Rollup of 8 pull requests Successful merges: - rust-lang#85285 (Add eslint checks to CI) - rust-lang#85709 (Use correct edition when parsing `:pat` matchers) - rust-lang#85762 (Do not try to build LLVM with Zlib on Windows) - rust-lang#85770 (Remove `--print unversioned-files` from rustdoc ) - rust-lang#85781 (Add documentation for aarch64-apple-ios-sim target) - rust-lang#85801 (Add `String::extend_from_within`) - rust-lang#85817 (Fix a typo) - rust-lang#85818 (Don't drop `PResult` without handling the error) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

bors · 2021-05-30T22:47:56Z

⌛ Testing commit 558b073 with merge c1e8f3a...

GuillaumeGomez added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label May 14, 2021

rust-highfive assigned jsha May 14, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 14, 2021

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the eslint-check branch from cd062c4 to 6b09ee6 Compare May 14, 2021 12:17

jyn514 reviewed May 14, 2021

View reviewed changes

GuillaumeGomez force-pushed the eslint-check branch from 6b09ee6 to 55bec99 Compare May 14, 2021 12:50

This comment has been minimized.

Sign in to view

jsha requested changes May 14, 2021

View reviewed changes

GuillaumeGomez force-pushed the eslint-check branch from c910aaa to 4950580 Compare May 14, 2021 15:36

GuillaumeGomez force-pushed the eslint-check branch from c5c2a85 to 4950580 Compare May 14, 2021 17:53

jsha reviewed May 14, 2021

View reviewed changes

src/librustdoc/html/static/source-script.js Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the eslint-check branch from 4950580 to dcf85c3 Compare May 14, 2021 20:23

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 14, 2021

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 14, 2021

GuillaumeGomez mentioned this pull request May 15, 2021

Fix eslint errors #85323

Merged

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 15, 2021

Rollup merge of rust-lang#85323 - GuillaumeGomez:fix-eslint-errors, r…

53c9f91

…=jsha Fix eslint errors I cherry-picked the two non-CI commits from rust-lang#85285. r? `@jsha`

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 15, 2021

Rollup merge of rust-lang#85323 - GuillaumeGomez:fix-eslint-errors, r…

7a8374e

…=jsha Fix eslint errors I cherry-picked the two non-CI commits from rust-lang#85285. r? ``@jsha``

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 15, 2021

Rollup merge of rust-lang#85323 - GuillaumeGomez:fix-eslint-errors, r…

46bc552

…=jsha Fix eslint errors I cherry-picked the two non-CI commits from rust-lang#85285. r? ```@jsha```

GuillaumeGomez force-pushed the eslint-check branch from dcf85c3 to c9b4c5a Compare May 15, 2021 15:18

GuillaumeGomez added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 15, 2021

rust-highfive assigned Mark-Simulacrum and unassigned jsha May 28, 2021

jyn514 added the A-testsuite Area: The testsuite used to check the correctness of rustc label May 29, 2021

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 29, 2021

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 30, 2021

GuillaumeGomez added 2 commits May 30, 2021 20:19

Add eslint checks in CI

b4148e9

Fix eslint error in sidebar-items.js

558b073

GuillaumeGomez force-pushed the eslint-check branch from c9b4c5a to 558b073 Compare May 30, 2021 18:21

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 30, 2021

GuillaumeGomez mentioned this pull request May 30, 2021

Rollup of 8 pull requests #85838

Merged

bors merged commit 9c873c1 into rust-lang:master May 30, 2021

rustbot added this to the 1.54.0 milestone May 30, 2021

GuillaumeGomez deleted the eslint-check branch May 31, 2021 07:45

Add eslint checks to CI #85285

Add eslint checks to CI #85285

Conversation

GuillaumeGomez commented May 14, 2021

rust-highfive commented May 14, 2021

This comment has been minimized.

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

jsha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented May 14, 2021

jsha commented May 14, 2021

GuillaumeGomez commented May 14, 2021

jsha commented May 14, 2021

bors commented May 14, 2021

jyn514 commented May 14, 2021 • edited Loading

GuillaumeGomez commented May 15, 2021

GuillaumeGomez commented May 15, 2021

GuillaumeGomez commented May 28, 2021

Mark-Simulacrum commented May 29, 2021

bors commented May 29, 2021

bors commented May 30, 2021

This comment has been minimized.

bors commented May 30, 2021

GuillaumeGomez commented May 30, 2021

bors commented May 30, 2021

bors commented May 30, 2021

jyn514 commented May 14, 2021 •

edited

Loading