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

REF: Refactor to avoid circular dependencies #2831

Closed
jbrockmendel opened this Issue Feb 9, 2019 · 7 comments

Comments

Projects
None yet
2 participants
@jbrockmendel
Copy link
Contributor

jbrockmendel commented Feb 9, 2019

Moving off-topic part of discussion in #1628.

The learning curve for newbies (this guy!) would be easier if the dependency structure between modules were simplified. Some low-hanging fruit I would like to refactor:

  • Cython.Utils - The only intra-cython import in this file is from Cython.Compiler.Scanning import FileSourceDescriptor for search_include_directories, which is only used once in Compiler.Main. I propose to move search_include_directories to Main (or a new file with scope TBD) so that Cython.Utils is unambiguously dependency-free.

  • Compiler.Lexicon and Compiler.Scanning have circular imports that can be resolved by just putting the contents of Lexicon into Scanning and removing Lexicon.py

  • CompilationOptions and default_options should be moved from Main to Options.

    • CmdLine would then depend only on Options and not need runtime import from Main. Main's runtime import from CmdLine could be moved to the module level.
    • CompilationOptions.create_context would be removed and be replaced by a Context.from_options classmethod.
  • (lower priority) a Compiler.util directory could hold dependency-free StringEncoding, Naming, Future, DebugFlags.

    • Possibly TreePath just to get it out of the way.
    • Error would also be nice to add to that list, but it depends on Options. Possibly try to avoid that dependency.
  • At this point

    • CmdLine depends only on Options
    • Scanning depends only on util and Errors
  • (The connection to #1628) Context could have a parent class refactored out that defines only enough methods/attributes to pass to PyrexScanner inside the parse method. (The following line inside parse passes the PyrexScanner to Parsing.p_module, and I'm not yet sure what Context attributes that expects to exist, so I'm not sure whether Context.parse can be entirely refactored out)

@scoder

This comment has been minimized.

Copy link
Contributor

scoder commented Feb 9, 2019

move search_include_directories to Main

Seems reasonable. Although all the "search directory for whatever" functionality might merit its own module at some point. It's fairly complex (and probably needs to be, at least a little). It's a good start, though.

putting the contents of Lexicon into Scanning

No, but I think it should work to move the Method class into Actions.py (including its .pxd declaration in Scanning.pxd), because that is essentially what it is, a scanner action.

CompilationOptions and default_options should be moved from Main to Options

There are probably some uncomfortable details somewhere that escape me right now, but this sounds like a good idea. There are basically four types of options: the debug flags and the global ones in Options.py, then the compile options set from the default_options dict, and the directives (which currently fill most of Options.py). I can well live with having the DebugFlags out of sight, but the others can nicely live together.

a Compiler.util directory could hold …

  • Future – That's a compiler feature, not a utility. I think it should be visible. Maybe goes together with compiler options, at best.
  • Naming is part of the code generation, helping to avoid naming conflicts. Doesn't feel like a utility.
  • DebugFlags isn't really a utility thing either, it's more like the options, just less end user facing.
  • StringEncoding is the most "utility-like" on your list. It's a part of the compiler infrastructure, with functionality for parsing, string value handling and code generation. Also not necessarily something to move out of the way, I guess.
  • TreePath, ok, that's mostly test support.
  • Error, also compiler infrastructure, I'd say.

Overall, I don't really see enough to put into a "utility" sub-package. I rather see a functional split between "scanner/parser", "syntax tree nodes", and "transformations". Potentially also the "type system" (PyrexTypes + Buffer + MemoryView + maybe Builtin), which is really what makes Cython a programming language. But I don't think it's a good idea to split up the package right now, when we might end up with a long-living Py2 legacy branch where any difficulty to cherry-pick changes will the a major annoyance. That's a decision to postpone until we have decided when and in which version to cut off the Py2 support. If we do it after the next release, then doing any restructuring before that would actually be helpful. Although, flat is better than nested, right? ;)

@jbrockmendel

This comment has been minimized.

Copy link
Contributor Author

jbrockmendel commented Feb 9, 2019

Overall, I don't really see enough to put into a "utility" sub-package.

Totally reasonable. Unless there is a better shorthand available, I'm going to keep referring to these modules collectively as util for the purpose of saying "X depends only on util and Y".

Although, flat is better than nested, right?

Can't argue with the principle.

I'll put together a PR with the parts of this that you're on board with. Later I might make a proof of concept for other parts, in particular the Context refactor.

@scoder

This comment has been minimized.

Copy link
Contributor

scoder commented Feb 9, 2019

Regarding the Method "Action", here is what happens when building the lexicon. It gets wrapped in a Call Action. I think it would be best to actually make it inherit from Action instead and implement .perform() and .same_as() directly.

@scoder

This comment has been minimized.

Copy link
Contributor

scoder commented Feb 10, 2019

I have working code for the Method refactoring. Can I push it, or would it get in the way of anything you're doing right now?

@jbrockmendel

This comment has been minimized.

Copy link
Contributor Author

jbrockmendel commented Feb 10, 2019

Go for it. Next PR I have in mind will be to move CompilationOptions.

scoder added a commit that referenced this issue Feb 10, 2019

Make the "Method" scanner action a direct feature of the embedded Ple…
…x, instead of hacking it back into the Lexicon with a stray import.

See GH-2831.
@scoder

This comment has been minimized.

Copy link
Contributor

scoder commented Feb 11, 2019

Can we close this ticket, or is there something you would like to keep it open for?

@jbrockmendel

This comment has been minimized.

Copy link
Contributor Author

jbrockmendel commented Feb 11, 2019

This can be closed. I've been trying to simplify things further by moving a few low-dependency classes out of Compiler.Code but have hit a wall there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment