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

Factor back-end attribute checking into the back end. #80

Merged
merged 19 commits into from
May 2, 2023

Conversation

reventlov
Copy link
Collaborator

This change splits up front_end/attribute_checker.py, moving the generic parts to a new file util/attribute_util.py, and moving the back-end-specific parts to back_end/cpp/header_generator.py.

Some tests from front_end/attribute_checker_test.py were moved to a new test suite, header_generator_test.py. There should probably be a util/attribute_checker_test.py, but for now the old tests provide sufficient coverage.

As a result of moving some attribute checking into the back end, generate_header() can now return errors. A future change should convert some of the assert statements in the same file into error returns.

In order for the emboss_codegen_cpp.py driver to properly display errors, the original source code of the .emb is now included verbatim in the IR, increasing the IR size by about 3%.

This change does allow attributes from unknown back ends to pass with no checking -- if you put an attribute [(my_kewl_back_end) i_love_ducks: "ducks are awesome!"] in an .emb file, that will now pass through the compiler without complaint.

This change splits up `front_end/attribute_checker.py`, moving the
generic parts to a new file `util/attribute_util.py`, and moving the
back-end-specific parts to `back_end/cpp/header_generator.py`.

Some tests from `front_end/attribute_checker_test.py` were moved to
a new test suite, `header_generator_test.py`.  There should probably
be a `util/attribute_checker_test.py`, but for now the old tests
provide sufficient coverage.

As a result of moving some attribute checking into the back end,
`generate_header()` can now return errors.  A future change should
convert some of the `assert` statements in the same file into error
returns.

In order for the `emboss_codegen_cpp.py` driver to properly display
errors, the original source code of the `.emb` is now included
verbatim in the IR, increasing the IR size by about 3%.

This change does allow attributes from unknown back ends to pass
with no checking -- if you put an attribute `[(my_kewl_back_end)
i_love_ducks: "ducks are awesome!"]` in an `.emb` file, that will
now pass through the compiler without complaint.
@AaronWebster
Copy link
Collaborator

Would it be useful to the user to include an warning if the backend is unknown? It's not apparent what errors would be presented to the user in this case, especially with future non-c++ codegens.

@AaronWebster AaronWebster reopened this Apr 3, 2023
@AaronWebster
Copy link
Collaborator

LGTM

Fix `traverse_ir_test` by using `varkw` instead of `kwonlyargs`.
@reventlov
Copy link
Collaborator Author

Would it be useful to the user to include an warning if the backend is unknown? It's not apparent what errors would be presented to the user in this case, especially with future non-c++ codegens.

I don't have a good solution to this one (and it is the main reason I hadn't moved back end attribute checking into the back end earlier). The two least-bad solutions I've come up with are:

  • A command-line option that lets you specify a list of valid back ends for the .emb that you're compiling -- the problem is that this gets annoying if you're compiling an .emb from a third party.
  • A top-level "allowed back ends" attribute, with a default value of (for now) "cpp" or (soon) "cpp,proto". This probably isn't too bad, although it feels a bit hacky to me.

In both cases, the front end would only be able to check that the cpp in [(cpp) name: "value"] wasn't misspelled -- it wouldn't be able to check that name and "value" were valid.

This change creates the `emboss_library` and `cc_emboss_library`
Starlark rules, which mirror `proto_library` and `cc_proto_library`.

This also fixes issues with `.emb` files that import `.emb` files that
import `.emb` files, where imports-of-imports were not always included
in the compilation.

This also required new command-line arguments to `emboss_front_end`
(`--output-file`) and `emboss_codegen_cpp` (`--input-file` and
`--output-file`).
This change splits up `front_end/attribute_checker.py`, moving the
generic parts to a new file `util/attribute_util.py`, and moving the
back-end-specific parts to `back_end/cpp/header_generator.py`.

Some tests from `front_end/attribute_checker_test.py` were moved to
a new test suite, `header_generator_test.py`.  There should probably
be a `util/attribute_checker_test.py`, but for now the old tests
provide sufficient coverage.

As a result of moving some attribute checking into the back end,
`generate_header()` can now return errors.  A future change should
convert some of the `assert` statements in the same file into error
returns.

In order for the `emboss_codegen_cpp.py` driver to properly display
errors, the original source code of the `.emb` is now included
verbatim in the IR, increasing the IR size by about 3%.

This change does allow attributes from unknown back ends to pass
with no checking -- if you put an attribute `[(my_kewl_back_end)
i_love_ducks: "ducks are awesome!"]` in an `.emb` file, that will
now pass through the compiler without complaint.
This change splits up `front_end/attribute_checker.py`, moving the
generic parts to a new file `util/attribute_util.py`, and moving the
back-end-specific parts to `back_end/cpp/header_generator.py`.

Some tests from `front_end/attribute_checker_test.py` were moved to
a new test suite, `header_generator_test.py`.  There should probably
be a `util/attribute_checker_test.py`, but for now the old tests
provide sufficient coverage.

As a result of moving some attribute checking into the back end,
`generate_header()` can now return errors.  A future change should
convert some of the `assert` statements in the same file into error
returns.

In order for the `emboss_codegen_cpp.py` driver to properly display
errors, the original source code of the `.emb` is now included
verbatim in the IR, increasing the IR size by about 3%.

This change does allow attributes from unknown back ends to pass
with no checking -- if you put an attribute `[(my_kewl_back_end)
i_love_ducks: "ducks are awesome!"]` in an `.emb` file, that will
now pass through the compiler without complaint.
This change splits up `front_end/attribute_checker.py`, moving the
generic parts to a new file `util/attribute_util.py`, and moving the
back-end-specific parts to `back_end/cpp/header_generator.py`.

Some tests from `front_end/attribute_checker_test.py` were moved to
a new test suite, `header_generator_test.py`.  There should probably
be a `util/attribute_checker_test.py`, but for now the old tests
provide sufficient coverage.

As a result of moving some attribute checking into the back end,
`generate_header()` can now return errors.  A future change should
convert some of the `assert` statements in the same file into error
returns.

In order for the `emboss_codegen_cpp.py` driver to properly display
errors, the original source code of the `.emb` is now included
verbatim in the IR, increasing the IR size by about 3%.

This change does allow attributes from unknown back ends to pass
with no checking -- if you put an attribute `[(my_kewl_back_end)
i_love_ducks: "ducks are awesome!"]` in an `.emb` file, that will
now pass through the compiler without complaint.
This change splits up `front_end/attribute_checker.py`, moving the
generic parts to a new file `util/attribute_util.py`, and moving the
back-end-specific parts to `back_end/cpp/header_generator.py`.

Some tests from `front_end/attribute_checker_test.py` were moved to
a new test suite, `header_generator_test.py`.  There should probably
be a `util/attribute_checker_test.py`, but for now the old tests
provide sufficient coverage.

As a result of moving some attribute checking into the back end,
`generate_header()` can now return errors.  A future change should
convert some of the `assert` statements in the same file into error
returns.

In order for the `emboss_codegen_cpp.py` driver to properly display
errors, the original source code of the `.emb` is now included
verbatim in the IR, increasing the IR size by about 3%.

This change does still enforce some minimal checking of back end
attributes: the back end must be listed in the new
`[expected_back_ends]` attribute (default value `"cpp"`), or they are
considered to be an error.  This change does not document the
`[expected_back_ends]` attribute because it is not currently useful for
end users.
@reventlov reventlov merged commit 5b38126 into google:master May 2, 2023
1 check passed
@reventlov reventlov deleted the extract_attribute_checking branch May 2, 2023 06:55
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

2 participants