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

refactor: front-end type system #2974

Merged

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Jul 18, 2022

What I did

refactor type system. redo the type hierarchy - instead of distinction between "Primitives" and "Definitions", have single type system + ExprInfo

  • simplify type hierarchy
  • merge old and new type systems will do this in a follow up PR
  • refactor builtins dir structure. merge builtin_functions/ and builtin_interfaces/ to builtins/.

How I did it

rethinking of the approach; don't need two classes for each type, factor into VyperType + ExprInfo + VarInfo. handling of types in the regular namespace is generally by dispatching into special constructor (for structs and interfaces) or attributes (for enums) mode.

How to verify it

Commit message

This commit refactors the front-end type system.

Currently, there are essentially two classes types for each vyper type.
"Primitives" represented raw types, and "Definitions" represent
the instantiation of types attached to each expression in a program.
"Definitions" additionally carry around annotation information related
to the expressions they tag, such as mutability and location info. This
system also handled dispatching into the correct routines for cases
where a raw type (e.g. "MyStruct") is used in expr land (vs an instance
of a type, e.g.  "my_struct"). This is important because certain types
are callable or attributable - specifically, constructors and enum
members (ex. `MyStruct()` and `MyEnum.FOO`).

This commit reworks the system by factoring the annotation info into
VyperType and ExprInfo. "Primitives" in the old system can be thought of
as VyperTypes, and "Definitions" in the old system are most similar to
the new ExprInfo. To handle cases where types can live in expr land,
this commit also clarifies that usage by promoting raw types into the
special, internal TYPE_T type (which can be thought of as "the type of
a type"). This commit also simplifies the inheritance structure of vyper
types.

Some other miscellaneous things in this commit:
- AST class hierarchy gets more refined - addition of ExprNode
- rename of `vyper/semantics/validation/` -> `vyper/semantics/analysis/`
- type system directory structure simplified
- merge of `builtin_functions/` and `builtin_interfaces/` to `builtins/`

Co-authored-by: z80 <z80dev@mlnl.finance>

Description for the changelog

Cute Animal Picture

image

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 18, 2022

This pull request introduces 13 alerts and fixes 2 when merging 53de3d1 into 1d1ef5d - view on LGTM.com

new alerts:

  • 9 for Unused import
  • 3 for Unused local variable
  • 1 for First parameter of a method is not named 'self'

fixed alerts:

  • 1 for Conflicting attributes in base classes
  • 1 for Missing call to `__init__` during object initialization

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 18, 2022

This pull request introduces 10 alerts and fixes 2 when merging 516a8f7 into 1d1ef5d - view on LGTM.com

new alerts:

  • 6 for Unused import
  • 3 for Unused local variable
  • 1 for First parameter of a method is not named 'self'

fixed alerts:

  • 1 for Conflicting attributes in base classes
  • 1 for Missing call to `__init__` during object initialization

merge builtin_functions/ and builtin_interfaces/ to builtins/
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Nov 27, 2022

This pull request introduces 1 alert and fixes 3 when merging 1e10958 into 2d18e60 - view on LGTM.com

new alerts:

  • 1 for `__eq__` not overridden when adding attributes

fixed alerts:

  • 1 for Conflicting attributes in base classes
  • 1 for Missing call to `__init__` during object initialization
  • 1 for 'import *' may pollute namespace

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

Copy link
Contributor

@skellet0r skellet0r left a comment

Choose a reason for hiding this comment

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

Chad work ser.
I definitely like the reorganization done, adds clarity.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Nov 27, 2022

This pull request fixes 3 alerts when merging 7e565bd into 2d18e60 - view on LGTM.com

fixed alerts:

  • 1 for Conflicting attributes in base classes
  • 1 for Missing call to `__init__` during object initialization
  • 1 for 'import *' may pollute namespace

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

@fubuloubu fubuloubu changed the title chore: refactor type system refactor: update type system hierarchy Nov 27, 2022
vyper/semantics/analysis/base.py Show resolved Hide resolved
@charles-cooper charles-cooper changed the title refactor: update type system hierarchy refactor: front-end type system Nov 27, 2022
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Nov 27, 2022

This pull request fixes 3 alerts when merging 7a7a339 into 2d18e60 - view on LGTM.com

fixed alerts:

  • 1 for Conflicting attributes in base classes
  • 1 for Missing call to `__init__` during object initialization
  • 1 for 'import *' may pollute namespace

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

@charles-cooper charles-cooper enabled auto-merge (squash) November 27, 2022 17:52
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Nov 27, 2022

This pull request fixes 3 alerts when merging c7e23c4 into 2d18e60 - view on LGTM.com

fixed alerts:

  • 1 for Conflicting attributes in base classes
  • 1 for Missing call to `__init__` during object initialization
  • 1 for 'import *' may pollute namespace

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants