Skip to content

fix: options initializing with null input type#2464

Merged
Dorukyum merged 6 commits into
Pycord-Development:masterfrom
timedoutuk:master
Jun 23, 2024
Merged

fix: options initializing with null input type#2464
Dorukyum merged 6 commits into
Pycord-Development:masterfrom
timedoutuk:master

Conversation

@timedoutuk

@timedoutuk timedoutuk commented May 28, 2024

Copy link
Copy Markdown
Contributor

Summary

Previously, if for some reason an Option was created with a None input_type value, an error would not be raised until on_connect called sync_commands(). You can see the trouble I had to go through troubleshooting the vague error message here.

This two-line modification will simply raise an error if input_type is None after Option() has finished initialising.
If this is not a suitable fix, I am open to suggestions to make further modifications.

Information

  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed).
  • This PR is not a code change (e.g. documentation, README, typehinting,
    examples, ...).

Checklist

  • I have searched the open pull requests for duplicates.
  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • If type: ignore comments were used, a comment is also left explaining why.
  • I have updated the changelog to include these changes.

@timedoutuk timedoutuk changed the title Fix: Prevent Option.__init__() completing with a null input type fix: Prevent Option.__init__() completing with a null input type May 28, 2024
@timedoutuk

Copy link
Copy Markdown
Contributor Author

TIL Fix != fix :^)

Lulalaby
Lulalaby previously approved these changes May 28, 2024
@Lulalaby
Lulalaby requested review from JustaSqu1d and plun1331 and removed request for BobDotCom May 28, 2024 14:27
@Lulalaby
Lulalaby enabled auto-merge (squash) May 28, 2024 14:27
@plun1331

Copy link
Copy Markdown
Member

Is it possible we could fix the underlying cause instead? input_type should not be None unless the user set it to None
I believe it may have to do with this method:

input_type = SlashCommandOptionType.from_datatype(
enum_choices[0].value.__class__
)

It seems to be possible for it to return None, perhaps we should default it to a string type instead?

@timedoutuk

Copy link
Copy Markdown
Contributor Author

@plun1331 I was unsure what the root cause is and figured just patching it like this would suffice until the proper solution can actually be found. I can certainly play around and test a few things though.
If I fix the issue I'll update this PR

@timedoutuk

timedoutuk commented May 28, 2024

Copy link
Copy Markdown
Contributor Author

Okay so I just spent ages walking through Option.__init__ manually, and have found the issue. The processing is as follows:

  1. The function's argument list contains two or more discord.ApplicationContext or bridge.BridgeContext type annotations. (or, in my case, the first argument was unannotated, but assumed by the library to be context, and then my second argument was actually supposed to be context and annotated as so. If I had not annotated the second argument, it would've defaulted to a string)
  2. Only the first argument is expected to be a context (excluding in classes where it is self). As such, any arguments after are treated as options.
  3. The enum if block evaluates to False as, while input_type_is_class is evaluated to True, the type is not an instance of Enum or DiscordEnum. So we continue.
  4. We skip past the Converter if block as nContext is not a converter subclass, leading us to the else statement.
  5. The first thing that is done in that try/except/else is call from_datatype with the phony context type.
  6. Inside from_datatype, its observable that datatype is not a tuple, Member/User/nChannel/Role/Attachment/Mentionable or a standard datatype, skipping over the majority of the function.
  7. We get to the final statement which purely makes sure that if the type is not one of the contexts, that TypeError is raised.
  8. The function returns None as there is no explicit return, nor an else for the skipped if.
  9. We then get to Option's else of the try block as there was no raised error, however input_type is now None.
  10. No further processing is done in __init__, meaning the class successfully initialises with a NoneType input_type, as the rest of the references to it are merely equality comparisons, and you can compare None to anything.

@timedoutuk

Copy link
Copy Markdown
Contributor Author

Now that we know what is actually causing the error, I believe an error message is more appropriate than simply defaulting to a type of string or whatever, as clearly the developer is expecting some form of context - raising an error would very easily indicate that is not what they would receive.

@Dorukyum

Copy link
Copy Markdown
Member

I'd take this as a temporary fix, although we are aware that the option handling system still requires heavy changes.

@timedoutuk

Copy link
Copy Markdown
Contributor Author

although we are aware that the option handling system still requires heavy changes

Aside from a general tidy up, what sort of changes does it really need?

@Dorukyum

Copy link
Copy Markdown
Member

Aside from a general tidy up, what sort of changes does it really need?

A very thorough tidy up.

Signed-off-by: Dorukyum <[email protected]>
@timedoutuk

Copy link
Copy Markdown
Contributor Author

The docs links CI keeps failing, I don't think that's my fault is it? There's so many CI jobs to look through

@timedoutuk

Copy link
Copy Markdown
Contributor Author

Any movement on this?

@plun1331

plun1331 commented Jun 4, 2024

Copy link
Copy Markdown
Member

The docs links CI keeps failing, I don't think that's my fault is it? There's so many CI jobs to look through

CI looks good on my end, everything passed

@timedoutuk

Copy link
Copy Markdown
Contributor Author

CI looks good on my end, everything passed

Turns out I was misreading the notification github was giving me, it was giving me an error on my fork itself, not the PR. All is good here.

@Dorukyum Dorukyum changed the title fix: Prevent Option.__init__() completing with a null input type fix: options initializing with null input type Jun 22, 2024
@Dorukyum

Copy link
Copy Markdown
Member

Upon further thought, I think we should try to solve the root issue instead. @plun1331's idea of defaulting the option type to string seems like a good solution.

@timedoutuk

timedoutuk commented Jun 22, 2024

Copy link
Copy Markdown
Contributor Author

Upon further thought, I think we should try to solve the root issue instead. @plun1331's idea of defaulting the option type to string seems like a good solution.

I do still disagree with this as the user would be expecting a different type, which if then used in the code would raise an AttributeError (for example, trying .defer()) - I still think preventing the option from initialising at all would be significantly easier and more helpful to debug (since the error is obviously coming from parameters provided, not a bug in the library giving "unexpected" inputs), and prevents unexpected behaviour.

Although, if the consensus is a string default, I can make that change and update this PR 👍

Dorukyum
Dorukyum previously approved these changes Jun 23, 2024

@Dorukyum Dorukyum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I just realised what the actual issue is. This happens when you add an argument before ctx when you shouldn't, therefore the program should throw an error.

@timedoutuk

Copy link
Copy Markdown
Contributor Author

I just realised what the actual issue is. This happens when you add an argument before ctx when you shouldn't, therefore the program should throw an error.

I investigated the entire logic flow, hence why I'm adamant it should be an error, not a default 👍

@Dorukyum
Dorukyum disabled auto-merge June 23, 2024 21:28
@Dorukyum
Dorukyum merged commit 284d40c into Pycord-Development:master Jun 23, 2024
baribarton pushed a commit to baribarton/pycord-no-potential-reconnect that referenced this pull request Oct 24, 2024
* Prevent Option.__init__() completing with a null input type

* Update changelog (for the checkbox)

* style(pre-commit): auto fixes from pre-commit.com hooks

* Update CHANGELOG.md

Signed-off-by: Dorukyum <[email protected]>

* style(pre-commit): auto fixes from pre-commit.com hooks

---------

Signed-off-by: Dorukyum <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Dorukyum <[email protected]>
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.

4 participants