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

Add Try node #1867

Merged
merged 3 commits into from
Jul 8, 2023
Merged

Add Try node #1867

merged 3 commits into from
Jul 8, 2023

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Nov 14, 2022

Description

Replaces both #1388 and #1389. I first tried implementing the migration plan laid out in #1388 (comment). However, the new Try node not being part of the "official" tree caused a lot of issues.

  • The most difficult part would have been to set the parent attributes on all child nodes correctly. Since it needs to point to the immediate parent, it would have been different depending if a child was part of the "old" TryExcept or the new Try node. That would basically mean that we couldn't reuse the child nodes and instead need to create new ones.
  • With that, it's not clear how we can resolve inference to (only) use the new ones while they are effectively still hidden behind the special try_node attribute.
  • All tree visitors in pylint would still use the old child nodes or might even emit all errors twice if both the old and new ones are visited.

I reached the conclusion that it might be better to do a breaking change now, instead of trying to work around a solution which would likely create new issues and would still lead to a breaking change down the line.

--
This PR replaces the old TryExcept and TryFinally nodes with the new Try node. Python replaced the old nodes a long time ago in 3.3. The new node simplifies things a bit and even helps generate better line numbers for pylint errors in try-except blocks. It will also make it much easier to add support for TryStar nodes which would just need to inherit from Try. pylint-dev/pylint#7703

--
As this PR is a (major) breaking change, I suggest it should be the start for astroid 3.0 once merged. I'm going to create a planning issue for 3.0. However, even with the breaking change, the work required to update is manageable. I'll also open a pylint PR to update our code base.

--

What's left to do

I'll also mention it in the 3.0 issue, but I think we should create a separate docs page just describing the breaking changes and how to update plugins. That can probably be done separately. If someone else would like to take the lead on this, I would appreciate any help. Some notes

  • TryExcept and TryFinally removed
  • Update visit_* and leave_* methods

This was referenced Nov 14, 2022
@cdce8p cdce8p added this to the 3.0 milestone Nov 14, 2022
@cdce8p cdce8p added Enhancement ✨ Improvement to a component High effort 🏋 Difficult solution or problem to solve ast labels Nov 14, 2022
@coveralls
Copy link

coveralls commented Nov 14, 2022

Pull Request Test Coverage Report for Build 3463412808

  • 35 of 35 (100.0%) changed or added relevant lines in 4 files are covered.
  • 6 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.07%) to 92.222%

Files with Coverage Reduction New Missed Lines %
astroid/nodes/_base_nodes.py 6 92.66%
Totals Coverage Status
Change from base Build 3463322631: -0.07%
Covered Lines: 9841
Relevant Lines: 10671

💛 - Coveralls

@DanielNoord
Copy link
Collaborator

@cdce8p Should this be reviewed? Or should we wait until we are sure we can start merging 3.0 PRs?

@cdce8p
Copy link
Member Author

cdce8p commented Nov 14, 2022

@cdce8p Should this be reviewed? Or should we wait until we are sure we can start merging 3.0 PRs?

The change itself is done. Might only need to resolve merge conflicts and update the changelog when we start with 3.0. If you have some spare time, feel free to take a look. Just don't merge it quite yet.

@cdce8p cdce8p added the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Nov 15, 2022
@codecov
Copy link

codecov bot commented Apr 23, 2023

Codecov Report

Merging #1867 (04ba8ca) into main (a7ab088) will decrease coverage by 0.07%.
The diff coverage is 97.72%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1867      +/-   ##
==========================================
- Coverage   92.84%   92.77%   -0.07%     
==========================================
  Files          94       94              
  Lines       10964    10941      -23     
==========================================
- Hits        10179    10151      -28     
- Misses        785      790       +5     
Flag Coverage Δ
linux 92.58% <97.72%> (-0.07%) ⬇️
pypy 90.89% <97.72%> (-0.07%) ⬇️
windows 92.34% <97.72%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
astroid/__init__.py 100.00% <ø> (ø)
astroid/nodes/__init__.py 100.00% <ø> (ø)
astroid/nodes/node_ng.py 93.67% <ø> (ø)
astroid/nodes/scoped_nodes/scoped_nodes.py 92.38% <ø> (ø)
astroid/nodes/node_classes.py 94.82% <97.43%> (-0.02%) ⬇️
astroid/nodes/as_string.py 97.01% <100.00%> (ø)
astroid/rebuilder.py 98.44% <100.00%> (+0.13%) ⬆️

... and 1 file with indirect coverage changes

@cdce8p cdce8p mentioned this pull request Apr 24, 2023
@Pierre-Sassoulas Pierre-Sassoulas mentioned this pull request Apr 24, 2023
8 tasks
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

It's not a work in progress anymore, right ? I think it's going to simplify a lot of checks in pylint 👍

astroid/nodes/node_classes.py Show resolved Hide resolved
@cdce8p cdce8p modified the milestones: 3.0.0a1, 3.0.0a2, 3.0.0a3 Apr 25, 2023
@cdce8p
Copy link
Member Author

cdce8p commented Apr 25, 2023

It's not a work in progress anymore, right ? I think it's going to simplify a lot of checks in pylint 👍

The PR is basically ready at this point. We could merge it as is.
The real work will be to update pylint after that. I do already have a PR with most of the required changes but we'll definitely need to double check these pylint-dev/pylint#7767.

Once we merge it, I'll create another alpha release just for it.

Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Great! Eager to merge this after we release 3.0.0a7 and can devote an alpha release to just this change.

@Pierre-Sassoulas
Copy link
Member

Let's do it ! :)

@Pierre-Sassoulas Pierre-Sassoulas merged commit e91a3b5 into pylint-dev:main Jul 8, 2023
17 of 18 checks passed
@jacobtylerwalls jacobtylerwalls modified the milestones: 3.0.0a8, 3.0.0a7 Jul 8, 2023
@cdce8p cdce8p deleted the try-node-breaking branch July 8, 2023 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ast breaking-change Enhancement ✨ Improvement to a component High effort 🏋 Difficult solution or problem to solve Needs review 🔍 Needs to be reviewed by one or multiple more persons
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants