Add package declaration in declarative part #307

Chris44442 · 2024-05-30T14:58:22Z

This is addressing the first two tasks of #225

Chris44442 · 2024-05-30T19:41:07Z

This is what I came up with. I am not sure if the changes in analysis/design_unit.rs are sensible. I am still not quite 100% what is going on in that particular code region tbh lol. Oh and I only added one test. Not sure if we need more.

Schottkyc137 · 2024-05-31T13:48:10Z

So far things look good to me. One thing to be wary of is that the package declaration might not be visible in the global scope. In other words,

package foo is
begin
  -- ...
end package foo;

Would mean that foo can be imported globally (I.e. use foo.bar would make bar visible) whereas

entity baz

package foo is
begin
  -- ...
end package foo;

end baz;

would probably mean that foo can only be accessed from within baz. I'm unsure what the LRM says in this regard though.

Also, I think that we should have more tests. The main purpose of the thorough testing is for regressions, I.e. checking that things also work after auxiliary changes.

I'm somewhat busy at the moment, so I can only give any further, code specific review later; latest however Sunday.

That being said thanks again for taking up this issue!

Chris44442 · 2024-05-31T14:12:50Z

Alright thanks, I will have a look at your notes later this day.

I have also pushed the Disconnection Specification to my fork. It now parses, but analysing Disconnect still does nothing for now. I will try to figure out how the analyse stuff works next, so I can try to implement it. The thing with Disconnect though is that it also depends on Guarded, which only seems partially implemented at the moment.

vhdl_lang/src/analysis/declarative.rs

Schottkyc137 · 2024-06-01T12:30:17Z

vhdl_lang/src/analysis/declarative.rs

+                self.analyze_package(unit, diagnostics)?;
+            }
+            Declaration::PackageBody(ref mut unit) => {
+                self.analyze_package_body(unit, diagnostics)?;


This (and the package declaration analysis) is fine, but I think that this declares them in the global namespace. As I have written in the second comment, check that this is correct.

I will try to resolve this next tomorrow or so. It is not exactly easy to interpret what the LRM really means, I have to say lol.

Yea, one of the hardest parts of developing a VHDL language server is interpreting the LRM :P

vhdl_lang/src/analysis/names.rs

vhdl_lang/src/ast.rs

vhdl_lang/src/ast/search.rs

vhdl_lang/src/named_entity.rs

vhdl_lang/src/syntax/declarative_part.rs

Schottkyc137 · 2024-06-01T12:45:00Z

vhdl_lang/src/syntax/design_unit.rs

+        assert_eq!(tokens[15].kind, End);
+        assert_eq!(tokens[16].kind, Package);
+        assert_eq!(tokens[17].kind, Body);
+    }


This test also doesn't do much. It just checks that the tokeniser works, but this is already tested elsewhere. Here, you should check that the AST matches, i.e., that if you parse that snippet, you will find an architecture declaration with package and package body inside. I think you can also omit the entity and just parse the architecture.

I think I need a little help here. I have tried to do what you said but I find it very hard to fill a default ArchitectureBody with the necessary values. The other tests use this simple_architecture, but I don't think this helps me much. I have put the test I thought of in comments.

vhdl_lang/src/syntax/disconnection.rs

Schottkyc137

All of this looks really good so far.
I have made comments where you had TODO marks and where I think some improvements could be made.

vhdl_lang/src/analysis/tests/declarations.rs

vhdl_lang/src/ast.rs

Schottkyc137 · 2024-06-02T18:52:01Z

vhdl_lang/src/syntax/design_unit.rs

+    //            span: code.token_span(),
+    //            context_clause: ContextClause::default(),
+    //            ident: code.s1("arch").decl_ident(),
+    //            entity_name: None, // TODO


This is just the name of the entity, i.e. ent in this example. You should simply be able to get a reference to that using WithRef::new(code.s1("ent").ident())

vhdl_lang/src/syntax/disconnection.rs

vhdl_lang/src/ast/search.rs

Add package declaration in declarative part

dba05d4

Chris44442 force-pushed the add_declarative_package_declaration branch from e094cc2 to dba05d4 Compare May 30, 2024 15:04

Chris44442 marked this pull request as draft May 30, 2024 16:00

Add package body in declarative part

d260d93

Chris44442 force-pushed the add_declarative_package_declaration branch from ee9a1af to d260d93 Compare May 30, 2024 17:19

Add package body and declaration remaining functionality

1ab2839

Chris44442 force-pushed the add_declarative_package_declaration branch from 042dfbe to 1ab2839 Compare May 30, 2024 19:33

Add disconnect specification

10cbd3d