IDLE: Fix and update and cleanup pyparse #77061
Comments
|
Pyparse was mostly written in the early 2000s, with the only one code change since 2007 in 2014. bpo-32874 will add tests for pyparse 'as is' (though with some docstring and comment changes). This issue will change pyparse code, and change or add test as needed. Here are two items to fix. More will be added. Some fixes might be separate PRs or spun off into separate issues. def dump: dump this; print does the same, without hardcoding sys.__stdout__, which might be None. _synchre: Only used in find_good_parse_start. Missing 'if' (mentioned in function docstring as 'popular' and 'for' and new things like 'with'. 'async' and 'await' are not generally popular, but is not any statement beginning a good parse start? |
|
Let's consider the todo questions at the end of the class Parser code. # XXX - is this used?
lastopenbracketpos = None
def get_last_open_bracket_pos(self):
"Return index of last open bracket or None."
self._study2()
return self.lastopenbracketpos
# XXX - is this used?
stmt_bracketing = None
def get_last_stmt_bracketing(self):
"""Return a tuple of the structure of the bracketing of the last
interesting statement.--- get_last_open_bracket_pos is not called anywhere. TODO remove it. Integer lastopenbracketpos is only conditionally set as an instance attribute, which masks the class attribute, in _study2 with However, self.lastopenbracketpos in only accessed (as 'j') in compute_bracket_indent, which is only called when a statement is continued to the next line because of an open bracket. Indeed, the call is guarded by Stopping here would break the current tests. We could make _study2 return the index and change test according. Or we can make _study2 always set the attribute with (TODO this) Since _study2 unconditionally sets stmt_bracketing as an instance attribute with |
|
set_str sets self.str and self.study_level. After the first call, attempts to access unset instance parse attributes from other modules will raise AttributeError. Removing the unneeded class setting will just add 2 more names to the list of things that should not be called directly. The get_xyz functions (get_continuation_type is another) avoid AttributeError (unless self.str is not set) and ensure validity. If set_str is called more than once, instance parse attributes will initially be set but invalid. So it seems they should be either deleted or set to None. In the latter case, they should also be set to None initially. I will look more at how the module is used. |
|
StringTranslatePseudoMapping (Mapping) is an awkward and not very descriptive name. It is similar to collections.defaultdict except that it uses a default value rather than a default factory. The latter is so defaults can be mutables. Would a subclass of similar to defaultdict be faster? Unlike, defaultdict, the initialized default is used in get as well as __getitem__. This means that STPM.get ignores a passed-in default, contrary to the definition of Mapping.get. The method override is not needed. The internal self._get is only needed because it is called in both __getitem__ and get. Without the get override, it's body (with 'self.' added, can become the body of __getitem__. I suspect that this would make __getitem__ faster. Test this. |
|
In def find_good_parse_start: |
|
Is And are the name bindings faster that regex.func(...)? |
|
In _study2, let q instead of p be end of last good line and then set p back to beginning. Setting p to end, q to p, and p to start is confusing. |
As noted in the test for find_good_parse_start and PR5755 discussion, a single line header on a line by itself in a multiline comment before a multiline header can prevent recognition of the latter.
>>> P.set_str("'somethn=ig'\ndef g(a,\nb\n")
>>> P.find_good_parse_start(lambda i: False)
13
>>> P.set_str("'''\ndef f():\n pass'''\ndef g(a,\nb\n")
>>> P.find_good_parse_start(lambda i:i < 15)
>>>One alternative to the current algorithm would be to search the beginning of every line for a compound statement keyword, not just lines ending with ':'. I believe the concern is that this would require uselessly checking more lines within strings. I believe that the same concern is why 'if' and 'for' are missing from the keyword list. When the window is an editor rather than shell, find_good_parse_start is called in EditorWindow.newline_and_indent_event and Hyperparser.__init__. The call-specific in-string function is returned by EW._build_char_in_string_func. It calls EW.is_char_in_string, which returns False only if the char in the text widget has been examined by the colorizer and not tagged with STRING. The call to find_good_parse_start is always followed by a call to set_lo and and then _study1 (via a call to another function). _study1 replaces runs of non-essential chars with 'x', which means that string literals within the code string are mostly reduced to a single x per line. (It would be fine if they were emptied except for newlines.) This suggests starting find_good_parse_start with a partial reduction, of just string literals, saved for further reduction by _study, so that keywords would never occur within the reduced literal. The problem is that one cannot tell for sure whether ''' or """ is the beginning or end of a multiline literal without parsing from the beginning of the code (which colorizer does). An alternate way to reuse the colorizer work might be to use splitlines on the code and then get all string tag ranges. The code-context option picks out compound-statement header lines. When enabled, I believe that its last line may be the desired good parse start line. Any proposed speedup should be tested by parsing multiple chunks of multiple stdlib modules. |
|
One sub-issue is to expand the current short doc section (2.1. Automatic indentation) the intended smart indenting. There is nothing about open parameter lists, not about multiline tuple/list/dict displays. It should say (IN CAPS?) that an indent that seems wrong likely indicates a syntax error that has mislead the indenter. |
|
Could this code be used in IDLE? For example highlighting the closed/open brace when cursor stay on open/closed brace or shortcuts for "go to open/closed brace"? Many editors have such functionality. |
|
The ()[]{} match manager is in parenmatch.py. It imports Hyperparser, which uses pyparse.Parser. When one types a closer, the corresponding opener is also highlighted and, depending on the configured style, everything in between. Very handy for showing when enough closers have been typed. Edit => Show surrounding parens, ^0 on Windows, which is essentially ^) on US keyboards, highlights out to the nearest pair. One can select one end of a pair by putting the cursor just inside a fence char. (A tk Text cursor is between characters, not on.) I don't think I would like automatic flashes when merely moving the cursor, or maybe I don't understand what you propose. On the hand, moving to the nearest opener to the left or closer to the right could be useful. ^] is already used for indent region, but ^{ and ^} (control-shift-bracket) are available and mnemonic. What shortcuts do you know of? |
|
Terry, In order to not duplicate effort, are you going to be working on these or is it ok if I look at them? Should all the changes happen in one PR or do you want to break them down? Thanks! |
|
Please go ahead, but coordinate. This issue contains miscellaneous pyparse issues that I thought of while reviewing the tests, plus Serhiy's idea. There should be multiple PRs on dependency issues (which might have more than one closely related PR). bpo-32905 and now bpo-32916 are examples. If you open a spinoff, I will assume that you are working on a PR. |
|
New issue: find_good_parse_start() call in editor.py has the wrong signature. There's actually a few things going on here. Here's the section in editor:
sends 3 parameters. And in pyparse, the signature allows 3. However, the signature is: This means that the False self.use_context_ps1 is the first value instead of the function, so pyparse is always executing: In the test, I had assumed this was for the default of None. Bad assumption. :-( Here's the commit that changed the signature: |
|
For msg312444 on StringTranslatePseudoMapping: I ran a comparison of the current class vs defaultdict. This was my test: I ran the current I know time.time isn't perfect, but thought it might be good enough. The results: Then I changed it to a defaultdict: I compared the results to make sure the defaultdict produced the same output as the mapping, which it did. The results: So, the results are simliar, down to the alternating of .61ish and .81ish times. |
|
I wish I could delete my last message (and duplicate posting of the one before. On the last one, I didn't move my timings. I feel like an idiot. Anyway, there is a difference. translate time: 7.443669319152832 translate time: 1.7251780033111572 It's still probably negligible since it's only done once and not a million times. :-) |
|
For msg312474 regarding replace vs re. Running the 20,000 copy version of the translated text (only translating once gives the following timings: And the re version gives: So, it's a little slower on a big string. It also gives slightly different results because of the last replace: On a more practical size document, they are about the same: re: It appears the time for the replace is linear and the re is just a little more than linear. Maybe it's just my computer. |
|
Looking at the creation of the instances of pyparse.PyParse() and hyperparser.HyperParser(), I was a little surprised that they (the instances) are local variables to the methods and aren't instance variables. Since they are called fairly often, wouldn't it be more efficient to use an instance variable (for example self.hp in calltips instead of hp) and update the attributes when something like open_calltips() is executed? Maybe the overhead of creating a class is neglible compared to the storage of not destroying it every time? |
|
If you [view] a message, there is an unlink button at the bottom for the author (and believe) and tracker gardeners. I did this for the duplicate and incomplete messages. In the future, lets put timing data for an idea on a separate issue for the idea. This puts everything about the idea in a one place. It is OK if we close the separate issue as rejected because the data show the idea not worthwhile. Unfortunately, there is no way to just move a message to another issue. But it can be copied to another and unlinked from the first. |
|
As I mentioned before, Parser.set_code strongly suggests that Parser instances were intended to be reused. But you discovered the bug that prevented this, at least for one code result. With that fixed, it should be possible to have one Parser instance per editor. Since hyperparser is used to match each ), ], and } we could look at that too. |
from idlelib.pyparse import Parser
import timeit
code='def f():\n'
print(timeit.timeit("statement", # for example
"p=Parser(4,4)", globals = globals()))statement microseconds Translate time is longer for real code and Parser() instance creation time must be well less than 1/10, maybe 1/100 of any answer time. If we don't reuse instances, though, set_code should be part of __init__ (and tests changed). Either change is low priority. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: