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

clippy: fix result_unit_err warnings #31791

Merged
merged 4 commits into from Mar 21, 2024
Merged

Conversation

eerii
Copy link
Contributor

@eerii eerii commented Mar 20, 2024

This pr tackles the clippy warnings about having a function returning a Result<_, ()>. There are three main cases:

  • The function returns a result directly from an external library (components/url/lib.rs). An allow is added for this case because we have no control over that.
  • The Result type is better expressed as an Option (it is used to check if something is there or not, and there is no differentiation between error scenarios). The spec also says for some functions that "This will return either a TYPE, or nothing.", so using an Option may be more in line. I understand that for some of the cases this may be controversial, so please add a suggestion if you think a specific return should stay a Result.
  • In cases where it made sense, added an error type to the result so it is more clear why is it failing. This was done in components/net/cookie_storage.rs and components/shared/canvas/webgl_channel/mod.rs.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes are a part of Fix as many clippy problems as possible #31500
  • These changes do not require tests because they only fix warnings, they don't add new functionality

@mrobinson mrobinson added the T-linux-wpt-2020 Do a try run of the WPT label Mar 20, 2024
@github-actions github-actions bot removed the T-linux-wpt-2020 Do a try run of the WPT label Mar 20, 2024
Copy link

🔨 Triggering try run (#8364474423) for Linux WPT

Copy link

Test results for linux-wpt-layout-2020 from try job (#8364474423):

Flaky unexpected result (22)
  • TIMEOUT /FileAPI/url/url-in-tags-revoke.window.html (#19978)
    • TIMEOUT [expected FAIL] subtest: Opening a blob URL in a new window immediately before revoking it works.

      Test timed out
      

  • FAIL [expected PASS] /_mozilla/css/iframe/hide_and_show.html (#15265)
  • TIMEOUT [expected OK] /_webgl/conformance/glsl/misc/shader-with-non-reserved-words.html (#16216)
    • NOTRUN [expected PASS] subtest: Overall test
  • TIMEOUT [expected OK] /_webgl/conformance/uniforms/out-of-bounds-uniform-array-access.html (#26225)
    • NOTRUN [expected PASS] subtest: Overall test
  • OK /css/css-fonts/variations/at-font-face-font-matching.html (#20684)
    • PASS [expected FAIL] subtest: Matching font-weight: '500' should prefer '350 399' over '351 398'
    • PASS [expected FAIL] subtest: Matching font-stretch: '110%' should prefer '115% 116%' over '105%'
    • PASS [expected FAIL] subtest: Matching font-style: 'oblique 21deg' should prefer 'oblique 30deg 60deg' over 'oblique 40deg 50deg'
  • TIMEOUT /fetch/metadata/generated/css-images.sub.tentative.html (#29047)
    • PASS [expected TIMEOUT] subtest: background-image sec-fetch-site - HTTPS downgrade (header not sent)
  • OK /html/browsers/browsing-the-web/navigating-across-documents/empty-iframe-load-event.html (#29066)
    • FAIL [expected PASS] subtest: Check execution order from nested timeout

      assert_equals: Expected nested setTimeout to run second expected true but got false
      

    • FAIL [expected PASS] subtest: Check execution order on load handler

      assert_equals: Expected onload to run first expected false but got true
      

  • OK /html/browsers/history/the-history-interface/traverse_the_history_2.html (#21383)
    • PASS [expected FAIL] subtest: Multiple history traversals, last would be aborted
  • PASS [expected CRASH] /html/canvas/element/manual/drawing-text-to-the-canvas/canvas.2d.disconnected-font-size-math.html (#30063)
  • TIMEOUT [expected OK] /html/interaction/focus/the-autofocus-attribute/supported-elements.html (#24145)
    • TIMEOUT [expected FAIL] subtest: Host element with delegatesFocus including no focusable descendants should be skipped

      Test timed out
      

    • NOTRUN [expected FAIL] subtest: Area element should support autofocus
  • TIMEOUT [expected CRASH] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-1.html (#22647)
  • TIMEOUT [expected CRASH] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-1.html (#24066)
  • OK /html/semantics/forms/form-submission-0/multipart-formdata.window.html (#28725)
    • PASS [expected FAIL] subtest: multipart/form-data: 0x00 in name (normal form)
    • PASS [expected FAIL] subtest: multipart/form-data: \n in value (normal form)
  • OK [expected TIMEOUT] /html/semantics/forms/form-submission-0/reparent-form-during-planned-navigation-task.html (#29724)
    • PASS [expected TIMEOUT] subtest: reparent-form-during-planned-navigation-task
  • OK /html/semantics/forms/form-submission-0/text-plain.window.html (#28687)
    • PASS [expected FAIL] subtest: text/plain: \r\n in name (formdata event)
    • PASS [expected FAIL] subtest: text/plain: double quote in value (formdata event)
  • OK /html/semantics/forms/form-submission-0/urlencoded2.window.html (#28687)
    • PASS [expected FAIL] subtest: application/x-www-form-urlencoded: 0x00 in name (normal form)
    • PASS [expected FAIL] subtest: application/x-www-form-urlencoded: 0x00 in value (normal form)
  • OK /html/webappapis/dynamic-markup-insertion/document-write/module-delayed.html (#27659)
    • FAIL [expected PASS] subtest: async document.write in a module

      assert_true: onload must be called expected true got false
      

  • TIMEOUT /html/webappapis/scripting/events/compile-event-handler-settings-objects.html (#24246)
    • PASS [expected FAIL] subtest: The entry settings object while executing the compiled callback via Web IDL's invoke must be that of the node document
  • TIMEOUT [expected OK] /html/webappapis/scripting/processing-model-2/integration-with-the-javascript-job-queue/promise-job-entry.html (#25805)
    • TIMEOUT [expected FAIL] subtest: Fulfillment handler on pending-then-fulfilled promise

      Test timed out
      

    • TIMEOUT [expected FAIL] subtest: Rejection handler on pending-then-rejected promise

      Test timed out
      

  • OK /quirks/percentage-height-calculation.html
    • FAIL [expected PASS] subtest: The percentage height calculation quirk, #foo { height:100px } #test { height:100%; position:absolute }&lt;div id=foo&gt;&lt;div&gt;&lt;div id=test&gt;&lt;/div&gt;&lt;/div&gt;&lt;/div&gt;

      assert_equals: quirks mode expected "200px" but got "0px"
      

    • FAIL [expected PASS] subtest: The percentage height calculation quirk, #foo { height:100px } #test { height:100%; position:fixed }&lt;div id=foo&gt;&lt;div&gt;&lt;div id=test&gt;&lt;/div&gt;&lt;/div&gt;&lt;/div&gt;

      assert_equals: quirks mode expected "200px" but got "0px"
      

  • TIMEOUT [expected OK] /resource-timing/nested-context-navigations-iframe.html (#24311)
    • TIMEOUT [expected PASS] subtest: Test that cross-site iframe navigations are not observable by the parent, even after history navigations by the parent

      Test timed out
      

    • NOTRUN [expected PASS] subtest: Test that iframe navigations are not observable by the parent
    • NOTRUN [expected PASS] subtest: Test that crossorigin iframe navigations are not observable by the parent
    • NOTRUN [expected PASS] subtest: Test that cross-site iframe navigations are not observable by the parent
    • NOTRUN [expected PASS] subtest: Test that iframe refreshes are not observable by the parent
    • NOTRUN [expected PASS] subtest: Test that crossorigin iframe refreshes are not observable by the parent
    • NOTRUN [expected PASS] subtest: Test that cross-site iframe refreshes are not observable by the parent
  • TIMEOUT [expected OK] /webstorage/localstorage-about-blank-3P-iframe-opens-3P-window.partitioned.tentative.html (#29053)
    • TIMEOUT [expected PASS] subtest: StorageKey: test 3P about:blank window opened from a 3P iframe

      Test timed out
      

Stable unexpected results that are known to be intermittent (16)
  • FAIL [expected PASS] /_mozilla/css/dirty_viewport.html (#13731)
  • FAIL [expected PASS] /_mozilla/mozilla/iframe/resize_after_load.html (#13573)
  • TIMEOUT [expected OK] /_webgl/conformance/glsl/misc/shader-uniform-packing-restrictions.html (#28103)
    • NOTRUN [expected PASS] subtest: Overall test
  • PASS [expected FAIL] /css/css-sizing/dynamic-available-size-iframe.html (#31559)
  • OK /css/cssom-view/MediaQueryList-addListener-handleEvent.html (#24571)
    • FAIL [expected PASS] subtest: calls handleEvent method of event listener

      assert_equals: expected (object) object "[object Object]" but got (undefined) undefined
      

  • OK /css/cssom-view/MediaQueryList-addListener-removeListener.html (#24569)
    • PASS [expected FAIL] subtest: listeners are called correct number of times
  • TIMEOUT [expected OK] /html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/iframe-src-aboutblank-navigate-immediately.html (#29048)
    • TIMEOUT [expected FAIL] subtest: Navigating to a different document with link click

      Test timed out
      

    • NOTRUN [expected FAIL] subtest: Navigating to a different document with form submission
  • OK /html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/load-pageshow-events-window-open.html (#28691)
    • FAIL [expected PASS] subtest: load event does not fire on window.open('about:blank')

      assert_unreached: load should not be fired Reached unreachable code
      

  • OK /html/browsers/browsing-the-web/navigating-across-documents/navigation-unload-same-origin.window.html (#29049)
    • PASS [expected FAIL] subtest: Same-origin navigation started from unload handler must be ignored
  • OK /html/browsers/browsing-the-web/navigating-across-documents/replace-before-load/a-click.html (#28697)
    • PASS [expected FAIL] subtest: aElement.click() before the load event must NOT replace
  • OK /html/browsers/history/the-history-interface/traverse_the_history_5.html (#21383)
    • FAIL [expected PASS] subtest: Multiple history traversals, last would be aborted

      assert_array_equals: Pages opened during history navigation expected property 1 to be 5 but got 3 (expected array [6, 5] got [6, 3])
      

  • FAIL [expected CRASH] /html/canvas/element/manual/text/canvas.2d.disconnected.html (#30063)
  • OK [expected CRASH] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-2.html (#22667)
    • FAIL [expected TIMEOUT] subtest: Check that popups from a sandboxed iframe escape the sandbox if allow-popups-to-escape-sandbox is used

      assert_equals: It came from a sandboxed iframe expected "null" but got "http://web-platform.test:8000"
      

  • OK [expected TIMEOUT] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3.html (#24057)
    • FAIL [expected TIMEOUT] subtest: Check that popups from a sandboxed iframe escape the sandbox if allow-popups-to-escape-sandbox is used

      assert_equals: It came from a sandboxed iframe expected "null" but got "http://web-platform.test:8000"
      

  • TIMEOUT [expected OK] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-3.html (#24066)
  • TIMEOUT /resource-timing/test_resource_timing.html (#25720)
    • FAIL [expected PASS] subtest: PerformanceEntry has correct name, initiatorType, startTime, and duration (img)

      assert_equals: expected 24962048 but got 24961792
      

Copy link

✨ Try run (#8364474423) succeeded.

Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, though maybe we can preserve context for errors in some cases:

components/canvas/canvas_paint_thread.rs Show resolved Hide resolved
components/net/fetch/methods.rs Outdated Show resolved Hide resolved
components/net/fetch/methods.rs Outdated Show resolved Hide resolved
eerii and others added 2 commits March 21, 2024 12:41
Co-authored-by: Martin Robinson <mrobinson@igalia.com>
Co-authored-by: Martin Robinson <mrobinson@igalia.com>
@mrobinson mrobinson added this pull request to the merge queue Mar 21, 2024
Merged via the queue into servo:main with commit da696b7 Mar 21, 2024
9 checks passed
@eerii eerii deleted the clippy_result branch March 21, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants