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

fs: use assert in fsCall argument checking #38519

Closed
wants to merge 2 commits into from
Closed

Conversation

pd4d10
Copy link
Contributor

@pd4d10 pd4d10 commented May 3, 2021

It seems to be a piece of dead code and could be safely removed

1. The fsCall function is only used within the FileHandle prototype methods
2. The second argument of every call is this, which ensures it's an instance of FileHandle

Updated, see #38519 (comment)

Refs: https://coverage.nodejs.org/coverage-68e6673224365120/lib/internal/fs/promises.js.html#L268

@github-actions github-actions bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels May 3, 2021
@targos
Copy link
Member

targos commented May 3, 2021

The throw can be triggered like this:

import { open } from 'fs/promises'
const handle = await open(new URL(import.meta.url))
handle.read.call({})

@aduh95
Copy link
Contributor

aduh95 commented May 3, 2021

The throw can be triggered like this:

import { open } from 'fs/promises'
const handle = await open(new URL(import.meta.url))
handle.read.call({})

We should have a test for this. @pd4d10 would you be interested in adding this to #38517?

@pd4d10
Copy link
Contributor Author

pd4d10 commented May 3, 2021

The throw can be triggered like this:

import { open } from 'fs/promises'
const handle = await open(new URL(import.meta.url))
handle.read.call({})

OK, get it. Also noticed that the close method uses a class property instead of a prototype method, to bind this and avoid it being changed.

One more question, what if we change all these prototype methods, such as read, write, and others, to class properties? Is there any side effect?

@pd4d10
Copy link
Contributor Author

pd4d10 commented May 3, 2021

We should have a test for this. @pd4d10 would you be interested in adding this to #38517?

OK, that's exactly what I thought.

@jasnell
Copy link
Member

jasnell commented May 3, 2021

Alternatively this could be changed to an assert()

In the user perspective, it's not an arguemnt type error. Replace it
with an `assert` expression.
@pd4d10
Copy link
Contributor Author

pd4d10 commented May 6, 2021

Alternatively this could be changed to an assert()

+1. In the user perspective, it's not an argument type error.

Replaced it with an assert expression and also added test cases.

lib/internal/fs/promises.js Outdated Show resolved Hide resolved
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

Could you also update the PR title?

@pd4d10 pd4d10 changed the title fs: remove unnecessary handle type checking fs: use assert in fsCall argument checking May 6, 2021
@pd4d10
Copy link
Contributor Author

pd4d10 commented May 6, 2021

Could you also update the PR title?

Updated

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 6, 2021

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 6, 2021
@jasnell
Copy link
Member

jasnell commented May 6, 2021

Landed in 8231cc4

@jasnell jasnell closed this May 6, 2021
jasnell pushed a commit that referenced this pull request May 6, 2021
In the user perspective, it's not an arguemnt type error. Replace it
with an `assert` expression.

PR-URL: #38519
Refs: https://coverage.nodejs.org/coverage-68e6673224365120/lib/internal/fs/promises.js.html#L268
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@pd4d10 pd4d10 deleted the patch-2 branch May 6, 2021 17:59
targos pushed a commit that referenced this pull request May 17, 2021
In the user perspective, it's not an arguemnt type error. Replace it
with an `assert` expression.

PR-URL: #38519
Refs: https://coverage.nodejs.org/coverage-68e6673224365120/lib/internal/fs/promises.js.html#L268
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants