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

[Dev] Allow top-level await in code statements #3508

Merged
merged 9 commits into from Feb 13, 2020
Merged

[Dev] Allow top-level await in code statements #3508

merged 9 commits into from Feb 13, 2020

Conversation

zephyrkul
Copy link
Contributor

@zephyrkul zephyrkul commented Feb 5, 2020

Type

  • Bugfix
  • Enhancement
  • New feature

Description of the changes

Allows for top-level await, async for, etc. code in debug and repl using the 3.8+ ast.PyCF_ALLOW_TOP_LEVEL_AWAIT flag.

The resulting object may be awaited on twice for backwards compatibility.

@zephyrkul zephyrkul requested a review from tekulvw as a code owner Feb 5, 2020
@Flame442 Flame442 added the Type: Enhancement label Feb 5, 2020
@zephyrkul
Copy link
Contributor Author

@zephyrkul zephyrkul commented Feb 5, 2020

Per Discord conversation, I've added asyncio and aiohttp to the env for [p]debug and [p]eval.

@jack1142 jack1142 added this to the 3.3.1 milestone Feb 5, 2020
@mikeshardmind
Copy link
Contributor

@mikeshardmind mikeshardmind commented Feb 5, 2020

This set of changes is on hold due to some exceptional weird behavior.

@mikeshardmind mikeshardmind removed this from the 3.3.1 milestone Feb 5, 2020
@mikeshardmind mikeshardmind added this to the 3.3.2 milestone Feb 5, 2020
@zephyrkul
Copy link
Contributor Author

@zephyrkul zephyrkul commented Feb 5, 2020

It seems like adding __builtins__ in 27b59dd resolved the exceptionally weird behavior. Based on the keys in locals() for stable's repl (['ctx', 'bot', 'message', 'guild', 'channel', 'author', '_', '__builtins__']), compared to the keys in this PR's repl (['ctx', 'bot', 'message', 'guild', 'channel', 'author', 'asyncio', '_', '__builtins__'], only asyncio is different here), this should theoretically fully resolve the main issue Sinbad found. However, because repl, and especially this version of repl, does some... interesting things with python, waiting for now is probably a good call.

As for the other, more minor issue that Sinbad found....
Screenshot

@mikeshardmind
Copy link
Contributor

@mikeshardmind mikeshardmind commented Feb 5, 2020

Some of the weirdness here is actually various known issues with python's asyncio repl.
As of the latest changes, this works as well as can be expected reasonably currently.

With that known, should we hold these until python 3.9, accept the weirdness with some warning, or have a separate command to enable to asyncio repl behavior?

(Technically, there are more clever approaches we could use such as some of the things jishaku does, but this is likely overkill for a cog we don't recommend exist in production)

@jack1142
Copy link
Member

@jack1142 jack1142 commented Feb 5, 2020

This is IMO great improvement even if it has some small issues so I don't think we should hold this of until Python 3.9. IMO, the best way here would be accepting this as a change to [p]repl and [p]debug, instead of making separate [p]arepl and [p]adebug commands. This should still let people run any code that they were able to run before and it's going to pretty much only improve the dev experience.

@zephyrkul
Copy link
Contributor Author

@zephyrkul zephyrkul commented Feb 5, 2020

Given that the weirdness is (apparently) acceptable for the built-in asyncio repl, I don't think that the weirdness is anything that particularly needs to be held off for py 3.9, especially for a cog that isn't for production.

@jack1142
Copy link
Member

@jack1142 jack1142 commented Feb 5, 2020

Running [p]debug [] results in error:

TypeError: object list can't be used in 'await' expression

@jack1142
Copy link
Member

@jack1142 jack1142 commented Feb 9, 2020

Value of __debug__ is inconsistent between eval and debug/repl - eval's __debug__ value is False when running red with python -O -m redbot and debug's and repl's __debug__ value is True.

@zephyrkul
Copy link
Contributor Author

@zephyrkul zephyrkul commented Feb 9, 2020

Value of __debug__ is inconsistent between eval and debug/repl - eval's __debug__ value is False when running red with python -O -m redbot and debug's and repl's __debug__ value is True.

I considered that out of scope of this PR, since this PR is focused more around adding await to repl and debug, but I can add this in if you insist.

@jack1142
Copy link
Member

@jack1142 jack1142 commented Feb 9, 2020

If we would consider this out of scope of this PR, then [p]debug and [p]repl should remain with the current (pre this PR) behaviour which is using same optimize mode as the one Red runs with.

@zephyrkul
Copy link
Contributor Author

@zephyrkul zephyrkul commented Feb 9, 2020

If we would consider this out of scope of this PR, then [p]debug and [p]repl should remain with the current behaviour which is using same optimize mode as the one Red runs with.

Sinbad agreed to keep the optimize mode as this PR has, so I haven't removed it. This PR is, again, mainly about improvements to debug and repl, so I didn't touch eval. But since you insist, I've added this to 62b0861.

Edit: To specify for clarification, the optimize changes were initially unintended, but kept in anyway after a conversation in discord.

@jack1142
Copy link
Member

@jack1142 jack1142 commented Feb 9, 2020

aiohttp is imported added to globals in [p]eval and [p]debug but not in [p]repl.

@zephyrkul
Copy link
Contributor Author

@zephyrkul zephyrkul commented Feb 9, 2020

aiohttp is imported added to globals in [p]eval and [p]debug but not in [p]repl.

On purpose. repl is meant to have a clean environment, or I assume it was meant to based on the fact that stable has the same "issue" with commands and discord.

@mikeshardmind
Copy link
Contributor

@mikeshardmind mikeshardmind commented Feb 9, 2020

Behavior here is largely consistent with prior behavior with the exception of optimize=0 (which was discussed)

I think the other changes we may consider now that we have people looking at this a bit closer with changes are fine to delay to other PRs.

@mikeshardmind mikeshardmind merged commit 42a2327 into Cog-Creators:V3/develop Feb 13, 2020
5 checks passed
@zephyrkul zephyrkul deleted the dev-async branch Feb 13, 2020
@zephyrkul zephyrkul mentioned this pull request Sep 4, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Dev Cog Type: Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants