fix(BitField): throw an error if bit to resolve is undefined

[kevinbioj]

Please describe the changes this PR makes and why it should be merged:

Currently, BitField.has(undefined) or BitField.has(<BitField>.FLAGS.ANY_NON_SPECIFIED_FLAG) returns true.

Status and versioning classification:

Please move lines that apply to you out of the comment:

• Code changes have been tested against the Discord API, or there are no code changes
• I know how to update typings and have done so, or typings don't need updating
• This PR changes the library's interface (methods or parameters added)
• This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
• This PR only includes non-code changes, like changes to documentation, README, etc.

[jzeuzs]

lgtm

[SpaceEEC]

I don't really think this is what we want.

Taking an example where this would yield an unexpected result: A command handler.

In our example no permissions would be required, maybe it's a ping command.
The handler wants to catch invalid permissions early, so it's resolving the required permissions (defaulting to an empty array) to 0n.
Because it's clever it's saving the number instead of the array. (Saving a few cpu cycles, performance and stuff)
Then when a member wants to run said command the handler checks for them using effectively message.member.permissions.has(0n) and gets false, thus denying the user to execute said command.
(Except when they are admin, but that's unrelated to this issue)

As result you'd now need to change your command handler to use something like requiredPermissions === Permissions.defaultBit || message.member.permissions.has(requiredPermissions).

bitField.has(0) should return true, since there are no bits not set in the bit field.

---

Another inconsistency here is that apparently identical values would yield different results
Here: bitField.has(0) with false and bitField.has([]) with true (Both would return 0 if passed to BitField.resolve)

i.e. bitField.has(BitField.resolve(n)) === bitField.has(n) should always return true (or both throw an error)

---

Maybe this issue here be approached by editing BitField.resolve, although I am pretty sure we internally rely on undefined resolving to BitField.defaultBit everywhere.

[kevinbioj]

I don't really think this is what we want.

Taking an example where this would yield an unexpected result: A command handler.

In our example no permissions would be required, maybe it's a ping command.
The handler wants to catch invalid permissions early, so it's resolving the required permissions (defaulting to an empty array) to 0n.
Because it's clever it's saving the number instead of the array. (Saving a few cpu cycles, performance and stuff)
Then when a member wants to run said command the handler checks for them using effectively message.member.permissions.has(0n) and gets false, thus denying the user to execute said command.
(Except when they are admin, but that's unrelated to this issue)

As result you'd now need to change your command handler to use something like requiredPermissions === Permissions.defaultBit || message.member.permissions.has(requiredPermissions).

bitField.has(0) should return true, since there are no bits not set in the bit field.

Another inconsistency here is that apparently identical values would yield different results
Here: bitField.has(0) with false and bitField.has([]) with true (Both would return 0 if passed to BitField.resolve)

i.e. bitField.has(BitField.resolve(n)) === bitField.has(n) should always return true (or both throw an error)

Maybe this issue here be approached by editing BitField.resolve, although I am pretty sure we internally rely on undefined resolving to BitField.defaultBit everywhere.

I'm not sure what to think anymore... Is this a "non-issue"? Or should we go back to the basic change where we check if bit is undefined (and incidentally if the array is empty, if one is provided).

Going back to the command handler example, I, in my opinion, think we should not call Permissions#has if we have no permissions to check against (at least that's my vision).

[kyranet]

Please address the issue explained by @SpaceEEC, as this current code contains a critical bug that will make it behave in unexpected ways.

[kevinbioj]

Please address the issue explained by @SpaceEEC, as this current code contains a critical bug that will make it behave in unexpected ways.

My main proposal was to return false whenever an unknown/undefined bit was given to the method (even tho I wasn't checking for possible undefined bits in an array). Vlad then requested to check, instead, if bit was equal to defaultBit after calling resolve since when undefined, the defaultBit is returned, leading to the noticed issue.
Now I see the problem in that case, but that does not tell me what would an acceptable solution be.

• Should <BitField>.constructor.resolve throw an error when typeof bit === undefined ?
• Should we silently ignore undefined bit(s) and do not count them ? (making <BitField>.has(undefined) === false)
• Should we keep the current behavior and therefore, close this PR.

[kevinbioj]

The first solution is probably the best one, I mean we should not give any answer if we are not able to give one.

• Reverted changes to BitField#has
• Updated static resolve method so if typeof bit === undefined it passes through all if and throws an error

[kevinbioj]

Need advice for the three last commits.
Needed so <Permissions>.has([..., undefined, ...]) throws an error.

[SpaceEEC]

Should be good to go otherwise.

[SpaceEEC]

I'm not sure what exactly this solves, but I'd rather drop it and the recursion that requires it.
BitField.resolve handles arrays just fine.

[kevinbioj]

Without this, it used to return true whenever they had admin no matter if you actually passed true to checkAdmin.
I still do not really understand the issue here, anyway as you said <BitField>.constructor.resolve handles arrays well already.

Stale