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.

Sign up for GitHub

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

Jump to bottom

[node] bigint options for fstat and lstat #50120

Merged
merged 4 commits into from
Feb 19, 2021
Merged

[node] bigint options for fstat and lstat #50120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a70fc84
[node] bigint options for fstat and lstat
xandris Dec 14, 2020
2d9ed05
[node] fixes for stat, fstat, lstat in all modes
xandris Dec 15, 2020
4eaafdc
[node] more stat/lstat/fstat overload tweaks, more tests
xandris Dec 15, 2020
1e39b21
node: fix some copypasta in v12
xandris Feb 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 28 additions & 13 deletions types/node/fs.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,70 +536,85 @@ declare module "fs" {
* Asynchronous stat(2) - Get file status.
* @param path A path to a file. If a URL is provided, it must use the `file:` protocol.
*/
export function stat(path: PathLike, options: BigIntOptions, callback: (err: NodeJS.ErrnoException | null, stats: BigIntStats) => void): void;
export function stat(path: PathLike, options: StatOptions, callback: (err: NodeJS.ErrnoException | null, stats: Stats | BigIntStats) => void): void;
export function stat(path: PathLike, callback: (err: NodeJS.ErrnoException | null, stats: Stats) => void): void;
export function stat(path: PathLike, options: StatOptions & { bigint?: false } | undefined, callback: (err: NodeJS.ErrnoException | null, stats: Stats) => void): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

There is quite some copy paste of this compound type.
What about adding something like this instead:

interface NoBigIntOptions {
  bigint?: false;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

It actually looks easier to see why this matters with the options inline. Are you recommending it be StatOptions & NoBigIntOptions? Or are you recommending a NoBigIntOptions interface that inherits from StatOptions? If the latter, The naming of BigIntOptions/NoBigIntOptions seems very ambiguous to me. Personally, I'd rather see either:

  1. StatOptions & { bigint: true } (as is written), or
  2. BigIntStatOptions/NoBigIntStatOptions (indicating the specific variations of StatOptions

Personally I think (1) is easier to read and reason over.

export function stat(path: PathLike, options: StatOptions & { bigint: true }, callback: (err: NodeJS.ErrnoException | null, stats: BigIntStats) => void): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the advantage of StatOptions & { bigint: true } compared to BigIntOptions used till now?

Copy link
Collaborator

@rbuckton rbuckton Feb 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It actually looks easier to see why this matters with the options inline.

export function stat(path: PathLike, options: StatOptions | undefined, callback: (err: NodeJS.ErrnoException | null, stats: Stats | BigIntStats) => void): void;
Copy link
Contributor

@Flarna Flarna Feb 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think the options === undefined case is not optimal here as the result should be of type Stats.

I think omitting | undefined here is better because this case is handled correct by the overload two lines above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Its possible you could have a weakly defined StatOptions | undefined that you want to pass in, which should be legal. Since you can't differentiate between whether its a Stats or BigIntStats due to the weaker type, I would expect that to be the result. This definition looks fine to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, the undefined case should hit the first overload (with & { bigint?: false }), which will correctly return Stats.


// NOTE: This namespace provides design-time support for util.promisify. Exported members do not exist at runtime.
export namespace stat {
/**
* Asynchronous stat(2) - Get file status.
* @param path A path to a file. If a URL is provided, it must use the `file:` protocol.
*/
function __promisify__(path: PathLike, options: BigIntOptions): Promise<BigIntStats>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should BigIntOptions be deprecated since it's not referenced in the API anymore?

function __promisify__(path: PathLike, options: StatOptions): Promise<Stats | BigIntStats>;
function __promisify__(path: PathLike): Promise<Stats>;
function __promisify__(path: PathLike, options?: StatOptions & { bigint?: false }): Promise<Stats>;
function __promisify__(path: PathLike, options: StatOptions & { bigint: true }): Promise<BigIntStats>;
function __promisify__(path: PathLike, options?: StatOptions): Promise<Stats | BigIntStats>;
}

/**
* Synchronous stat(2) - Get file status.
* @param path A path to a file. If a URL is provided, it must use the `file:` protocol.
*/
export function statSync(path: PathLike, options: BigIntOptions): BigIntStats;
export function statSync(path: PathLike, options: StatOptions): Stats | BigIntStats;
export function statSync(path: PathLike): Stats;
export function statSync(path: PathLike, options?: StatOptions & { bigint?: false }): Stats;
export function statSync(path: PathLike, options: StatOptions & { bigint: true }): BigIntStats;
export function statSync(path: PathLike, options?: StatOptions): Stats | BigIntStats;

/**
* Asynchronous fstat(2) - Get file status.
* @param fd A file descriptor.
*/
export function fstat(fd: number, callback: (err: NodeJS.ErrnoException | null, stats: Stats) => void): void;
export function fstat(fd: number, options: StatOptions & { bigint?: false } | undefined, callback: (err: NodeJS.ErrnoException | null, stats: Stats) => void): void;
export function fstat(fd: number, options: StatOptions & { bigint: true }, callback: (err: NodeJS.ErrnoException | null, stats: BigIntStats) => void): void;
export function fstat(fd: number, options: StatOptions | undefined, callback: (err: NodeJS.ErrnoException | null, stats: Stats | BigIntStats) => void): void;

// NOTE: This namespace provides design-time support for util.promisify. Exported members do not exist at runtime.
export namespace fstat {
/**
* Asynchronous fstat(2) - Get file status.
* @param fd A file descriptor.
*/
function __promisify__(fd: number): Promise<Stats>;
function __promisify__(fd: number, options?: StatOptions & { bigint?: false }): Promise<Stats>;
function __promisify__(fd: number, options: StatOptions & { bigint: true }): Promise<BigIntStats>;
function __promisify__(fd: number, options?: StatOptions): Promise<Stats | BigIntStats>;
}

/**
* Synchronous fstat(2) - Get file status.
* @param fd A file descriptor.
*/
export function fstatSync(fd: number): Stats;
export function fstatSync(fd: number, options?: StatOptions & { bigint?: false }): Stats;
export function fstatSync(fd: number, options: StatOptions & { bigint: true }): BigIntStats;
export function fstatSync(fd: number, options?: StatOptions): Stats | BigIntStats;

/**
* Asynchronous lstat(2) - Get file status. Does not dereference symbolic links.
* @param path A path to a file. If a URL is provided, it must use the `file:` protocol.
*/
export function lstat(path: PathLike, callback: (err: NodeJS.ErrnoException | null, stats: Stats) => void): void;
export function lstat(path: PathLike, options: StatOptions & { bigint?: false } | undefined, callback: (err: NodeJS.ErrnoException | null, stats: Stats) => void): void;
export function lstat(path: PathLike, options: StatOptions & { bigint: true }, callback: (err: NodeJS.ErrnoException | null, stats: BigIntStats) => void): void;
export function lstat(path: PathLike, options: StatOptions | undefined, callback: (err: NodeJS.ErrnoException | null, stats: Stats | BigIntStats) => void): void;

// NOTE: This namespace provides design-time support for util.promisify. Exported members do not exist at runtime.
export namespace lstat {
/**
* Asynchronous lstat(2) - Get file status. Does not dereference symbolic links.
* @param path A path to a file. If a URL is provided, it must use the `file:` protocol.
*/
function __promisify__(path: PathLike): Promise<Stats>;
function __promisify__(path: PathLike, options?: StatOptions & { bigint?: false }): Promise<Stats>;
function __promisify__(path: PathLike, options: StatOptions & { bigint: true }): Promise<BigIntStats>;
function __promisify__(path: PathLike, options?: StatOptions): Promise<Stats | BigIntStats>;
}

/**
* Synchronous lstat(2) - Get file status. Does not dereference symbolic links.
* @param path A path to a file. If a URL is provided, it must use the `file:` protocol.
*/
export function lstatSync(path: PathLike): Stats;
export function lstatSync(path: PathLike, options?: StatOptions & { bigint?: false }): Stats;
export function lstatSync(path: PathLike, options: StatOptions & { bigint: true }): BigIntStats;
export function lstatSync(path: PathLike, options?: StatOptions): Stats | BigIntStats;

/**
* Asynchronous link(2) - Create a new link (also known as a hard link) to an existing file.
Expand Down Expand Up @@ -2234,6 +2249,6 @@ declare module "fs" {
}

export interface StatOptions {
bigint: boolean;
bigint?: boolean;
}
}
20 changes: 11 additions & 9 deletions types/node/fs/promises.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
declare module 'fs/promises' {
import {
Stats,
BigIntStats,
StatOptions,
WriteVResult,
ReadVResult,
PathLike,
Expand Down Expand Up @@ -92,7 +94,9 @@ declare module 'fs/promises' {
/**
* Asynchronous fstat(2) - Get file status.
*/
stat(): Promise<Stats>;
stat(opts?: StatOptions & { bigint?: false }): Promise<Stats>;
stat(opts: StatOptions & { bigint: true }): Promise<BigIntStats>;
stat(opts?: StatOptions): Promise<Stats | BigIntStats>;

/**
* Asynchronous ftruncate(2) - Truncate a file to a specified length.
Expand Down Expand Up @@ -357,23 +361,21 @@ declare module 'fs/promises' {
*/
function symlink(target: PathLike, path: PathLike, type?: string | null): Promise<void>;

/**
* Asynchronous fstat(2) - Get file status.
* @param handle A `FileHandle`.
*/
function fstat(handle: FileHandle): Promise<Stats>;

/**
* Asynchronous lstat(2) - Get file status. Does not dereference symbolic links.
* @param path A path to a file. If a URL is provided, it must use the `file:` protocol.
*/
function lstat(path: PathLike): Promise<Stats>;
function lstat(path: PathLike, opts?: StatOptions & { bigint?: false }): Promise<Stats>;
function lstat(path: PathLike, opts: StatOptions & { bigint: true }): Promise<BigIntStats>;
function lstat(path: PathLike, opts?: StatOptions): Promise<Stats | BigIntStats>;

/**
* Asynchronous stat(2) - Get file status.
* @param path A path to a file. If a URL is provided, it must use the `file:` protocol.
*/
function stat(path: PathLike): Promise<Stats>;
function stat(path: PathLike, opts?: StatOptions & { bigint?: false }): Promise<Stats>;
function stat(path: PathLike, opts: StatOptions & { bigint: true }): Promise<BigIntStats>;
function stat(path: PathLike, opts?: StatOptions): Promise<Stats | BigIntStats>;

/**
* Asynchronous link(2) - Create a new link (also known as a hard link) to an existing file.
Expand Down
165 changes: 165 additions & 0 deletions types/node/test/fs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,3 +357,168 @@ async function testPromisify() {
fs.readv(123, [Buffer.from('wut')] as ReadonlyArray<NodeJS.ArrayBufferView>, 123, (err: NodeJS.ErrnoException | null, bytesRead: number, buffers: NodeJS.ArrayBufferView[]) => {
});
}

async function testStat(
path: string,
fd: number,
opts: fs.StatOptions,
maybeFalse: fs.StatOptions & { bigint: false } | undefined,
maybeTrue: fs.StatOptions & { bigint: true } | undefined,
maybe?: fs.StatOptions,
) {
/* Need to test these variants:
* - stat
* - lstat
* - fstat
*
* In these modes:
* - callback
* - sync
* - promisify
* - fs.promises
*
* With these opts:
* - None => Stats
* - Undefined => Stats
* - { } => Stats
* - { bigint: false } | undefined => Stats
/ - { bigint: true } => BigIntStats
/ - { bigint: true } | undefined => Stats | BigIntStats
* - { bigint: boolean } (can't infer) | undefined => Stats | BigIntStats
*
* Which is 3 x 4 x 7 = 84 tests
*
* (fs.promises.fstat doesn't exist, but those 6 cases are in FileHandle.fstat)
*/

// Callback mode
fs.stat(path, (err, st: fs.Stats) => {});
fs.lstat(path, (err, st: fs.Stats) => {});
fs.fstat(fd, (err, st: fs.Stats) => {});

fs.stat(path, undefined, (err, st: fs.Stats) => {});
fs.lstat(path, undefined, (err, st: fs.Stats) => {});
fs.fstat(fd, undefined, (err, st: fs.Stats) => {});

fs.stat(path, {}, (err, st: fs.Stats) => {});
fs.lstat(path, {}, (err, st: fs.Stats) => {});
fs.fstat(fd, {}, (err, st: fs.Stats) => {});

fs.stat(path, maybeFalse, (err, st: fs.Stats) => {});
fs.lstat(path, maybeFalse, (err, st: fs.Stats) => {});
fs.fstat(fd, maybeFalse, (err, st: fs.Stats) => {});

fs.stat(path, { bigint: true }, (err, st: fs.BigIntStats) => {});
fs.lstat(path, { bigint: true }, (err, st: fs.BigIntStats) => {});
fs.fstat(fd, { bigint: true }, (err, st: fs.BigIntStats) => {});

fs.stat(path, maybeTrue, (err, st) => {
st; // $ExpectType Stats | BigIntStats
});
fs.lstat(path, maybeTrue, (err, st) => {
st; // $ExpectType Stats | BigIntStats
});
fs.fstat(fd, maybeTrue, (err, st) => {
st; // $ExpectType Stats | BigIntStats
});

fs.stat(path, opts, (err, st) => {
st; // $ExpectType Stats | BigIntStats
});

fs.lstat(path, opts, (err, st) => {
st; // $ExpectType Stats | BigIntStats
});

fs.fstat(fd, opts, (err, st) => {
st; // $ExpectType Stats | BigIntStats
});

// Sync mode
fs.statSync(path); // $ExpectType Stats
fs.lstatSync(path); // $ExpectType Stats
fs.fstatSync(fd); // $ExpectType Stats

fs.statSync(path, undefined); // $ExpectType Stats
fs.lstatSync(path, undefined); // $ExpectType Stats
fs.fstatSync(fd, undefined); // $ExpectType Stats

fs.statSync(path, {}); // $ExpectType Stats
fs.lstatSync(path, {}); // $ExpectType Stats
fs.fstatSync(fd, {}); // $ExpectType Stats

fs.statSync(path, maybeFalse); // $ExpectType Stats
fs.lstatSync(path, maybeFalse); // $ExpectType Stats
fs.fstatSync(fd, maybeFalse); // $ExpectType Stats

fs.statSync(path, { bigint: true }); // $ExpectType BigIntStats
fs.lstatSync(path, { bigint: true }); // $ExpectType BigIntStats
fs.fstatSync(fd, { bigint: true }); // $ExpectType BigIntStats

fs.statSync(path, maybeTrue); // $ExpectType Stats | BigIntStats
fs.lstatSync(path, maybeTrue); // $ExpectType Stats | BigIntStats
fs.fstatSync(fd, maybeTrue); // $ExpectType Stats | BigIntStats

fs.statSync(path, opts); // $ExpectType Stats | BigIntStats
fs.lstatSync(path, opts); // $ExpectType Stats | BigIntStats
fs.fstatSync(fd, opts); // $ExpectType Stats | BigIntStats

// Promisify mode
util.promisify(fs.stat)(path); // $ExpectType Promise<Stats>
util.promisify(fs.lstat)(path); // $ExpectType Promise<Stats>
util.promisify(fs.fstat)(fd); // $ExpectType Promise<Stats>

util.promisify(fs.stat)(path, undefined); // $ExpectType Promise<Stats>
util.promisify(fs.lstat)(path, undefined); // $ExpectType Promise<Stats>
util.promisify(fs.fstat)(fd, undefined); // $ExpectType Promise<Stats>

util.promisify(fs.stat)(path, {}); // $ExpectType Promise<Stats>
util.promisify(fs.lstat)(path, {}); // $ExpectType Promise<Stats>
util.promisify(fs.fstat)(fd, {}); // $ExpectType Promise<Stats>

util.promisify(fs.stat)(path, maybeFalse); // $ExpectType Promise<Stats>
util.promisify(fs.lstat)(path, maybeFalse); // $ExpectType Promise<Stats>
util.promisify(fs.fstat)(fd, maybeFalse); // $ExpectType Promise<Stats>

util.promisify(fs.stat)(path, { bigint: true }); // $ExpectType Promise<BigIntStats>
util.promisify(fs.lstat)(path, { bigint: true }); // $ExpectType Promise<BigIntStats>
util.promisify(fs.fstat)(fd, { bigint: true }); // $ExpectType Promise<BigIntStats>

util.promisify(fs.stat)(path, maybeTrue); // $ExpectType Promise<Stats | BigIntStats>
util.promisify(fs.lstat)(path, maybeTrue); // $ExpectType Promise<Stats | BigIntStats>
util.promisify(fs.fstat)(fd, maybeTrue); // $ExpectType Promise<Stats | BigIntStats>

util.promisify(fs.stat)(path, opts); // $ExpectType Promise<Stats | BigIntStats>
util.promisify(fs.lstat)(path, opts); // $ExpectType Promise<Stats | BigIntStats>
util.promisify(fs.fstat)(fd, opts); // $ExpectType Promise<Stats | BigIntStats>

// fs.promises mode
const fh = await fs.promises.open(path, 'r');
fs.promises.stat(path); // $ExpectType Promise<Stats>
fs.promises.lstat(path); // $ExpectType Promise<Stats>
fh.stat(); // $ExpectType Promise<Stats>

fs.promises.stat(path, undefined); // $ExpectType Promise<Stats>
fs.promises.lstat(path, undefined); // $ExpectType Promise<Stats>
fh.stat(undefined); // $ExpectType Promise<Stats>

fs.promises.stat(path, {}); // $ExpectType Promise<Stats>
fs.promises.lstat(path, {}); // $ExpectType Promise<Stats>
fh.stat({}); // $ExpectType Promise<Stats>

fs.promises.stat(path, maybeFalse); // $ExpectType Promise<Stats>
fs.promises.lstat(path, maybeFalse); // $ExpectType Promise<Stats>
fh.stat(maybeFalse); // $ExpectType Promise<Stats>

fs.promises.stat(path, { bigint: true }); // $ExpectType Promise<BigIntStats>
fs.promises.lstat(path, { bigint: true }); // $ExpectType Promise<BigIntStats>
fh.stat({ bigint: true }); // $ExpectType Promise<BigIntStats>

fs.promises.stat(path, maybeTrue); // $ExpectType Promise<Stats | BigIntStats>
fs.promises.lstat(path, maybeTrue); // $ExpectType Promise<Stats | BigIntStats>
fh.stat(maybeTrue); // $ExpectType Promise<Stats | BigIntStats>

fs.promises.stat(path, opts); // $ExpectType Promise<Stats | BigIntStats>
fs.promises.lstat(path, opts); // $ExpectType Promise<Stats | BigIntStats>
fh.stat(opts); // $ExpectType Promise<Stats | BigIntStats>
}
Loading