Skip to content

Proposal: Refactor extract class AssignmentStatementAnalyzer from SemanticAnalyzer #2198

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.

Sign up for GitHub

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

Jump to bottom
Closed
davidfstr opened this issue Sep 30, 2016 · 3 comments
Closed

Proposal: Refactor extract class AssignmentStatementAnalyzer from SemanticAnalyzer #2198

davidfstr opened this issue Sep 30, 2016 · 3 comments

Comments

@davidfstr
Copy link
Contributor

davidfstr commented Sep 30, 2016 •
edited
Loading

I want to propose a refactoring that I will volunteer to execute. Before spending time on it, I would like to see if there are any high-level objections.

Implementation Proposal

(1) The following tree of methods will be extracted from SemanticAnalyzer and moved to a new class _AssignmentStatementAnalyzer that is used internally by SemanticAnalyzer.

    visit_assignment_stmt (public)
        analyze_simple_literal_type

        check_and_set_up_type_alias

        process_newtype_declaration
            analyze_newtype_declaration
            check_newtype_args
            build_newtype_typeinfo
                make_argument

        process_typevar_declaration
            check_typevar_name
            get_typevar_declaration
            analyze_types
            process_typevar_parameters

        process_namedtuple_definition
            check_namedtuple
                parse_namedtuple_args
                    parse_namedtuple_fields_with_types
                        fail_namedtuple_arg
                build_namedtuple_typeinfo

All methods in the new _AssignmentStatementAnalyzer class except visit_assignment_stmt() will be prefixed with underscore to indicate that they are private to the class.

(2) SemanticAnalyzer.visit_assignment_stmt() will be altered to delegate to _AssignmentStatementAnalyzer.visit_assignment_stmt().

Benefits

  • Total surface area of SemanticAnalyzer is reduced by:
    • 582 lines of code -- 25% of the original 2424 LOC in SemanticAnalyzer
    • 18 methods
  • Public surface area of the extracted method group is reduced to 1 method:
    • visit_assignment_stmt()

I believe that this change will significantly improve the readability of SemanticAnalyzer for future readers.
@davidfstr davidfstr mentioned this issue Sep 30, 2016
Closed
@gvanrossum
Copy link
Member

gvanrossum commented Sep 30, 2016 via email

@davidfstr
Copy link
Contributor Author

Okay then I won't volunteer to take this task on myself at the present time.

some uneasy back-and-forth between the two classes

Indeed there will be some. For any potential implementors who may be reading, one detail I left out is that that a small number of private trampoline methods will need to be added to _AssignmentStatementAnalyzer to assist in forwarding requests to the original SemanticAnalyzer that aren't handled internally by _AssignmentStatementAnalyzer.

If there are no other immediate implementor volunteers then I suggest closing this issue.

@davidfstr davidfstr mentioned this issue Dec 12, 2016
Merged
@elazarg
Copy link
Contributor

elazarg commented Dec 12, 2016

This refactoring is needed, although it's not exactly about AssignmentStatement (a nametuple might be a base class expression). Personally I'd move everything to a different module, not just a class - and perhaps no class is needed at all. This is of course related to #699.
I have actually done something very similar in a private branch, and the back-and-forth weren't such uneasy. Errors are reported using a simple callback or return values; there are very few dependencies otherwise.

Note that there are two independent tasks here: the parsing of "external" language constructs, and the semantic analysis of these constructs. The first task should IMO done before semantic analysis, as a pre-parsing phase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@davidfstr @gvanrossum @elazarg