Issue 375 add support for any type #380

ghaith · 2021-11-18T11:54:21Z

Generics/ANY type support:

Generics can be defined on functions
Functions with generics will be resolved to typed functions during the call
External declaration for typed functions are added
Generics implementation will be ignored in this commit.

codecov-commenter · 2021-11-18T12:02:21Z

Codecov Report

Merging #380 (a7946cc) into master (901304e) will increase coverage by 0.12%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master     #380      +/-   ##
==========================================
+ Coverage   92.89%   93.01%   +0.12%     
==========================================
  Files          31       31              
  Lines       10470    10924     +454     
==========================================
+ Hits         9726    10161     +435     
- Misses        744      763      +19

Impacted Files	Coverage Δ
src/ast.rs	`92.17% <77.77%> (-1.59%)`	⬇️
src/index.rs	`97.18% <86.95%> (+1.06%)`	⬆️
src/typesystem.rs	`94.80% <94.93%> (-0.78%)`	⬇️
src/codegen/generators/data_type_generator.rs	`94.55% <95.23%> (-0.11%)`	⬇️
src/resolver.rs	`95.95% <95.95%> (+0.01%)`	⬆️
src/parser.rs	`98.01% <97.72%> (-0.02%)`	⬇️
src/index/visitor.rs	`96.07% <98.66%> (+0.50%)`	⬆️
src/ast/pre_processor.rs	`100.00% <100.00%> (ø)`
src/codegen.rs	`100.00% <100.00%> (ø)`
src/codegen/generators/expression_generator.rs	`86.97% <100.00%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 901304e...a7946cc. Read the comment docs.

…rt_for_Any_type

riederm · 2021-11-24T19:55:31Z

src/ast.rs

+    GenericType {
+        name: String,
+        generic_symbol: String,
+        nature: String,


would this be usefull as an enum?

Do you mean the 'nature' field? So we could limit it to only possible ANY_XXX type natures? I think that might be good yes. I think once we're in OOP territory we are just working with interfaces / derived types anyway

sorry yes, I'm talking about the nature. I thought it would be highlighted when i mark it when commenting

src/parser.rs

…rt_for_Any_type

Avoid generating generic methods Actidate codegen tests for generics

…rt_for_Any_type

riederm · 2021-12-01T08:59:29Z

src/ast/pre_processor.rs


 pub fn pre_process(unit: &mut CompilationUnit) {
    //process all local variables from POUs
    for mut pou in unit.units.iter_mut() {
+        //Find all generic types in that pou


can we put this into it's own function?. It kindo'f disturbs the level of detail of the rest of this function

riederm · 2021-12-01T09:07:08Z

src/index/visitor.rs

+                name: enum_name.to_string(),
+                initial_value: init,
+                information,
+                nature: TypeNature::Any,


is enum really ANY

Do we have type casting for ENUM -> DINT? If yes they could be Signed otherwise we have to assume any or live with conversion issues if you call a generic ANY_INT method with an enum

riederm · 2021-12-01T09:08:14Z

src/index/visitor.rs

+                name: name.to_string(),
+                initial_value: init,
+                information,
+                nature: TypeNature::Any,


is subrange really any? It should be at least something numeric? no?

Subrange will / should never be queried directly, and at this stage I don't know the subtype of it, I could however in the least assume an Int nature so I'll change to that

riederm · 2021-12-01T09:20:32Z

src/lexer/tokens.rs

@@ -362,6 +362,9 @@ pub enum Token {
    #[regex("[a-zA-Z_][a-zA-Z_0-9]*#")]
    TypeCastPrefix,

+    #[regex("ANY(?:_(DERIVED|ELEMENTARY|MAGNITUDE|NUM|REAL|INT|SIGNED|UNSIGNED|DURATION|BIT|CHARS|STRING|CHAR|DATE)+)?", super::parse_type_nature)]


that's ok for me, although I would also not mind to not make them keywords, but to look for an identifier and check in the parser (so syntactical validation) that this a valid nature

I can indeed delegate to the parser, in which case we expect something like an Identifie and we manually check the possible types and create the nature
I'll delay this to the end though, i'm prioritizing the other issues first

riederm · 2021-12-01T09:28:19Z

src/ast.rs

@@ -35,6 +42,95 @@ pub enum DirectAccessType {
    DWord,
 }

+#[derive(Debug, PartialEq, Clone, Copy)]


i find it a bit surprising, that typeNatures are note defined in typesystem.rs

I guess you want to avoid the dependency from AST -> typesystem?

riederm · 2021-12-01T09:45:38Z

src/resolver.rs

+    /// It collects all candidates for a generic function
+    /// Then chooses the best fitting function signature
+    /// And reannotates the function with the found information
+    fn update_generic_information(


can we rename this to: update_generic_call_statement

riederm · 2021-12-01T09:49:32Z

src/resolver.rs

+    fn update_generic_information(
+        &mut self,
+        generics_candidates: HashMap<&str, Vec<&str>>,
+        operator_qualifier: &str,


operator_qualifier gives me a hard time. as far as I can see, we don't really use this a qualifier somewhere here?
is this the implementation_name maybe?

riederm · 2021-12-01T09:50:51Z

src/resolver.rs

+    fn update_generic_function_parameters(
+        &mut self,
+        s: &AstStatement,
+        operator_qualifier: &str,


operator_qualifier is this the function's name?

riederm · 2021-12-01T09:54:52Z

src/resolver.rs

+                            );
+                            self.annotation_map.add_generic_nature(p, *nature);
+                            //If assginment, annotate the left side with the new type
+                            if let AstStatement::Assignment { left, right, .. } = p {


usually the annotator does not annotate the assignment itself. semantically it is not correct that a := b reuslts in a's type - it results in void. there are languages that work like that, but so far rusty's ST-flavor does not.

3 lines above we annotated p, although it is an assignment.

i think we should only do this if it is not an Assignment. (so maybe just into an else-clause)

riederm · 2021-12-01T10:01:09Z

tests/correctness/generic_functions.rs

+}
+
+#[test]
+fn test_external_function_called() {


added additional tests that test if generic function parameters can be provided in an arbitrary order: it should make no difference if you write foo(a := x, b := y); or foo(b := y, a := x); for a foo<T, V>

riederm

👍 again, cool feature and nice implementation!

ghaith added 3 commits November 15, 2021 10:03

Add generics type to the AST

7750771

Add support for parsing generic types

e3fbdc1

Use vec instead of map, add generics to index

8f03a6b

ghaith force-pushed the issue-375-Add_support_for_Any_type branch from 935691c to 8f03a6b Compare November 18, 2021 11:57

ghaith and others added 4 commits November 18, 2021 13:17

Merge remote-tracking branch 'origin/master' into issue-375-Add_suppo…

265abfd

…rt_for_Any_type

Codegen tests

ccdb789

Add indexing support for generics

560eb00

Merge remote-tracking branch 'origin/master' into issue-375-Add_suppo…

9098079

…rt_for_Any_type

riederm reviewed Nov 24, 2021

View reviewed changes

ghaith and others added 13 commits November 27, 2021 15:10

Add resolver support for generics

8037f58

Merge remote-tracking branch 'origin/master' into issue-375-Add_suppo…

43a53a6

…rt_for_Any_type

Clippy fixes

276da9d

Code generation for generics

8b7bca3

Avoid generating generic methods Actidate codegen tests for generics

Add natures as keywords, add initial validation scaffold

fe7fdc4

Merge remote-tracking branch 'origin/master' into issue-375-Add_suppo…

9a54f4c

…rt_for_Any_type

Validation wip

71e8a41

Add validation for generics

aafb8e8

Deleted some demo files

fd7bb31

Add intermdiate index

d2b77fe

Merge remote-tracking branch 'origin/master' into issue-375-Add_suppo…

807a3b9

…rt_for_Any_type

Create implementation entries for resolved generics - wip

a598e23

Make sure types are generated, add correctness tests

c51bfa3

ghaith force-pushed the issue-375-Add_support_for_Any_type branch from 478a704 to c51bfa3 Compare December 1, 2021 06:59

ghaith marked this pull request as ready for review December 1, 2021 07:26

ghaith requested a review from riederm December 1, 2021 07:26

riederm requested changes Dec 1, 2021

View reviewed changes

riederm and others added 3 commits December 1, 2021 12:09

additional tests for generic-parameter annotation

4b8b977

added additional tests that test if generic function parameters can be provided in an arbitrary order: it should make no difference if you write foo(a := x, b := y); or foo(b := y, a := x); for a foo<T, V>

Fix some review comments, missing : Tokens

96ee719

Remove the keywords from the lexer and treat type natures as Identifiers

a7946cc

riederm approved these changes Dec 2, 2021

View reviewed changes

ghaith merged commit ca2ebac into master Dec 2, 2021

ghaith deleted the issue-375-Add_support_for_Any_type branch December 2, 2021 13:39

riederm mentioned this pull request Dec 2, 2021

Add support for "Any" type #375

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 375 add support for any type #380

Issue 375 add support for any type #380

ghaith commented Nov 18, 2021 •

edited

Loading

codecov-commenter commented Nov 18, 2021 •

edited

Loading

riederm Nov 24, 2021

ghaith Nov 25, 2021

riederm Nov 28, 2021

riederm Dec 1, 2021

riederm Dec 1, 2021

ghaith Dec 1, 2021

riederm Dec 1, 2021

ghaith Dec 1, 2021

riederm Dec 1, 2021

ghaith Dec 2, 2021

riederm Dec 1, 2021

riederm Dec 1, 2021

riederm Dec 1, 2021

riederm Dec 1, 2021

riederm Dec 1, 2021

ghaith Dec 2, 2021

riederm Dec 1, 2021

riederm left a comment

Issue 375 add support for any type #380

Issue 375 add support for any type #380

Conversation

ghaith commented Nov 18, 2021 • edited Loading

codecov-commenter commented Nov 18, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riederm left a comment

Choose a reason for hiding this comment

ghaith commented Nov 18, 2021 •

edited

Loading

codecov-commenter commented Nov 18, 2021 •

edited

Loading