rust_hdl icon indicating copy to clipboard operation
rust_hdl copied to clipboard

WIP: Implement analysis of formals in association lists

Open j-zeppenfeld opened this issue 3 years ago • 9 comments

This is very much a work in progress, but I want to share my initial efforts in order to avoid running in the wrong direction.

Everything I've tested works fine, but is much too lenient.

Some things that are yet to be solved:

  1. Interface lists do not have their own regions, so associating generics in the port map and vice-versa is not diagnosed.
  2. Indexed names are parsed as function calls due to syntax ambiguity. As a result Name::IndexedName is never instantiated, and indexed formals have to be disambiguated based on the function call's name.
  3. Type conversions and conversion functions are not checked for arity.
  4. Components do not currently have a region associated with themselves.

Feedback is welcome - if I am going in the wrong direction with this please let me know.

j-zeppenfeld avatar Mar 10 '21 21:03 j-zeppenfeld

First of all some general comments without looking at the code.

  1. That it is lenient is much better than giving false positives.
  2. It is missing test cases. For analysis there is analysis/tests module which contains different black box tests. You could create a new file there named resolve_formals with tests for the new functionality. Try to create tests during every step of the process so that the capabilities you add are reasonably covered with test. Ideally each iteration should contain new implementation and associated test.

kraigher avatar Mar 11 '21 15:03 kraigher

After looking at the code it looks like it is going in the right direction. The main thing it is missing is test cases. I typically add test cases for both cases that should be allowed and forbidden as I go. I also use the example_project with PoC, VUnit, OSVVM etc to validate that the new features does not give any false positives. If you run setup.sh to clone the example project you can run cargo run --bin vhdl_lang --release -- --config example_project/ to see that it runs cleanly.

kraigher avatar Mar 11 '21 15:03 kraigher

If you want inspiration for test you can look into resolves_names.rs.

kraigher avatar Mar 11 '21 16:03 kraigher

Thank you for the feedback! Tests are in the works, unfortunately I don't have much time to work on it this week, but I hope to make some progress over the coming weekend.

cargo run --bin vhdl_lang --release -- --config example_project/ gives a PermissionDenied error, I assume that accesses the directory and should be --config example_project/vhdl_ls.toml?

That gives the following errors out of the box:

error: No primary unit 'external_integer_vector_pkg' within library 'vunit_lib'
   --> C:\Projects\vhdl_ls\rust_hdl\example_project\vunit\vunit\vhdl\data_types\src\integer_vector_ptr_pkg.vhd:14
    |
12  |
13  |  use work.types_pkg.all;
14 --> use work.external_integer_vector_pkg.all;
    |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~
15  |
16  |  use work.codec_pkg.all;

error: No primary unit 'external_string_pkg' within library 'vunit_lib'
   --> C:\Projects\vhdl_ls\rust_hdl\example_project\vunit\vunit\vhdl\data_types\src\string_ptr_pkg.vhd:14
    |
12  |
13  |  use work.types_pkg.all;
14 --> use work.external_string_pkg.all;
    |           ~~~~~~~~~~~~~~~~~~~
15  |
16  |  use work.codec_pkg.all;

hint: Library clause not necessary for current working library
   --> C:\Projects\vhdl_ls\rust_hdl\example_project\UVVM\bitvis_vip_axilite\src\axilite_channel_handler_pkg.vhd:27
    |
25  |  context uvvm_util.uvvm_util_context;
26  |
27 --> library work;
    |          ~~~~
28  |  use work.axilite_bfm_pkg.all;
29  |

hint: Library clause not necessary for current working library
   --> C:\Projects\vhdl_ls\rust_hdl\example_project\UVVM\bitvis_vip_axilite\src\transaction_pkg.vhd:24
    |
22  |  context uvvm_util.uvvm_util_context;
23  |
24 --> library work;
    |          ~~~~
25  |  use work.axilite_bfm_pkg.all;
26  |

Adding 'vunit/vunit/vhdl/data_types/src/api/external_*.vhd', fixes the first two (errors), and I suppose the hints are indeed unclean code on the part of UVVM.

j-zeppenfeld avatar Mar 15 '21 20:03 j-zeppenfeld

Yes I meant the --config example_project/vhdl_ls.toml. Sorry about that. The example project vhdl_ls.toml file is probably inconsistent with the HEAD of the git submodules. The vhdl_ls.toml file reflects whatever version of PoC, VUnit etc I got last time it did git submodule update, which was a long time ago. We should really lock the git submodule versions of the example projects and update it in tandem with the vhdl_ls.toml.

kraigher avatar Mar 16 '21 15:03 kraigher

These are the revisions of the example projects that I have checked out:

UVVM: 27d54c0034d05b67099174e2f8a556e35cf921c1
PoC: 51248a8a85c05baa86be10399100871211d9f557
VUnit: 88f0d9ebb8fb9c3145ab5098604c1e7ddc53659d
OSVVM: f4567d6c300f1acb475ba785676e195c5d739ac4

kraigher avatar Mar 16 '21 15:03 kraigher

I pushed a commit to master which changes setup.sh to check out these version explicitly. It might be good to bump these eventually as they are quite old.

kraigher avatar Mar 16 '21 16:03 kraigher

@kraigher, why not add them as git submodules? Then, you would choose to retrieve them with option --recursive to git clone.

umarcor avatar Mar 17 '21 00:03 umarcor

Sure it is a good idea to use submodules

kraigher avatar Mar 17 '21 06:03 kraigher