Skip to content

Commit af83338

Browse files
authored
avoid re-registering generic implementations (#641)
when calling a generic function, we register an implementation and a POU in the index to later link against it. If multiple calls discover the same generic implementation (e.g. INT-implementation), they were added mulitple times to the index and the fix implemented in #354 caused a problem. ImplementationIndexEntry move back to IndexMap - we only keep one entry per name - there is no value since the POUIndexEntries handle the duplication validation. When importing POUIndexEntries we ignore automatically generated entries if they are already in the index. fixes #637
1 parent 44bad56 commit af83338

12 files changed

+344
-25
lines changed

src/codegen/generators/pou_generator.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ pub fn generate_implementation_stubs<'ink>(
5454
) -> Result<LlvmTypedIndex<'ink>, Diagnostic> {
5555
let mut llvm_index = LlvmTypedIndex::default();
5656
let pou_generator = PouGenerator::new(llvm, index, annotations, types_index);
57-
for (name, implementation) in index.get_implementations().elements() {
57+
for (name, implementation) in index.get_implementations() {
5858
if !implementation.is_generic() {
5959
let curr_f = pou_generator.generate_implementation_stub(implementation, module)?;
6060
llvm_index.associate_implementation(name, curr_f)?;
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
---
2+
source: src/codegen/tests/generics_test.rs
3+
expression: codegen(src)
4+
---
5+
; ModuleID = 'main'
6+
source_filename = "main"
7+
8+
%prg = type {}
9+
10+
@prg_instance = global %prg zeroinitializer
11+
12+
define void @prg(%prg* %0) {
13+
entry:
14+
%call = call i64 @CONCAT_DATE__INT(i16 1, i16 2, i16 3)
15+
%call1 = call i64 @CONCAT_DATE__DINT(i32 1, i32 2, i32 3)
16+
%call2 = call i64 @CONCAT_DATE__INT(i16 1, i16 2, i16 3)
17+
%call3 = call i64 @CONCAT_DATE__INT(i16 1, i16 2, i16 3)
18+
ret void
19+
}
20+
21+
declare i64 @CONCAT_DATE__INT(i16, i16, i16)
22+
23+
declare i64 @CONCAT_DATE__DINT(i32, i32, i32)
24+

src/index.rs

+46-6
Original file line numberDiff line numberDiff line change
@@ -404,6 +404,7 @@ pub enum PouIndexEntry {
404404
linkage: LinkageType,
405405
is_variadic: bool,
406406
location: SymbolLocation,
407+
is_generated: bool, // true if this entry was added automatically (e.g. by generics)
407408
},
408409
Class {
409410
name: String,
@@ -466,9 +467,6 @@ impl PouIndexEntry {
466467
}
467468

468469
/// creates a new Function-PouIndexEntry
469-
/// # Arguments
470-
/// - `name` the name of the function
471-
/// - `return_type` the function's return type
472470
pub fn create_function_entry(
473471
name: &str,
474472
return_type: &str,
@@ -485,6 +483,29 @@ impl PouIndexEntry {
485483
linkage,
486484
is_variadic,
487485
location,
486+
is_generated: false,
487+
}
488+
}
489+
490+
/// creates a new Function-PouIndexEntry generated by the compiler
491+
/// this will set the is_generated attribute to true.
492+
pub fn create_generated_function_entry(
493+
name: &str,
494+
return_type: &str,
495+
generic_names: &[GenericBinding],
496+
linkage: LinkageType,
497+
is_variadic: bool,
498+
location: SymbolLocation,
499+
) -> PouIndexEntry {
500+
PouIndexEntry::Function {
501+
name: name.into(),
502+
generics: generic_names.to_vec(),
503+
return_type: return_type.into(),
504+
instance_struct_name: name.into(),
505+
linkage,
506+
is_variadic,
507+
location,
508+
is_generated: true,
488509
}
489510
}
490511

@@ -690,6 +711,16 @@ impl PouIndexEntry {
690711
| PouIndexEntry::Class { location, .. } => location,
691712
}
692713
}
714+
715+
fn is_auto_generated_function(&self) -> bool {
716+
matches!(
717+
self,
718+
PouIndexEntry::Function {
719+
is_generated: true,
720+
..
721+
}
722+
)
723+
}
693724
}
694725

695726
/// the TypeIndex carries all types.
@@ -788,7 +819,9 @@ pub struct Index {
788819
pous: SymbolMap<String, PouIndexEntry>,
789820

790821
/// all implementations
791-
implementations: SymbolMap<String, ImplementationIndexEntry>,
822+
// we keep an IndexMap for implementations since duplication issues regarding implementations
823+
// is handled by the `pous` SymbolMap
824+
implementations: IndexMap<String, ImplementationIndexEntry>,
792825

793826
/// an index with all type-information
794827
type_index: TypeIndex,
@@ -906,7 +939,14 @@ impl Index {
906939
self.implementations.extend(other.implementations);
907940

908941
//pous
909-
self.pous.extend(other.pous);
942+
for (name, elements) in other.pous.drain(..) {
943+
for ele in elements {
944+
// skip automatically generated pou's if they are already in the target index
945+
if !ele.is_auto_generated_function() || !self.pous.contains_key(&name) {
946+
self.pous.insert(name.clone(), ele);
947+
}
948+
}
949+
}
910950

911951
//Constant expressions are intentionally not imported
912952
// self.constant_expressions.import(other.constant_expressions)
@@ -1299,7 +1339,7 @@ impl Index {
12991339
&self.enum_qualified_variables
13001340
}
13011341

1302-
pub fn get_implementations(&self) -> &SymbolMap<String, ImplementationIndexEntry> {
1342+
pub fn get_implementations(&self) -> &IndexMap<String, ImplementationIndexEntry> {
13031343
&self.implementations
13041344
}
13051345

src/index/tests/index_tests.rs

+1
Original file line numberDiff line numberDiff line change
@@ -1919,6 +1919,7 @@ fn a_program_pou_is_indexed() {
19191919
source_range: (65..75).into(),
19201920
line_number: 4
19211921
},
1922+
is_generated: false,
19221923
}),
19231924
index.find_pou("myFunction"),
19241925
);

src/resolver/generics.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -139,16 +139,16 @@ impl<'i> TypeAnnotator<'i> {
139139
);
140140

141141
//register a copy of the pou under the new name
142-
self.annotation_map
143-
.new_index
144-
.register_pou(PouIndexEntry::create_function_entry(
142+
self.annotation_map.new_index.register_pou(
143+
PouIndexEntry::create_generated_function_entry(
145144
new_name,
146145
return_type,
147146
&[],
148147
LinkageType::External, //it has to be external, we should have already found this in the global index if it was internal
149148
generic_function.is_variadic(),
150149
generic_function.get_location().clone(),
151-
));
150+
),
151+
);
152152

153153
// register the member-variables (interface) of the new function
154154
// copy each member-index-entry and make sure to turn the generic (e.g. T)

src/resolver/tests/resolve_generic_calls.rs

+10-10
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,9 @@ fn resolved_generic_call_added_to_index() {
3434
let (annotations, _) = TypeAnnotator::visit_unit(&index, &unit);
3535
// The implementations are added to the index
3636
let implementations = annotations.new_index.get_implementations();
37-
assert!(implementations.contains_key(&"myfunc__int".into()));
38-
assert!(implementations.contains_key(&"myfunc__dint".into()));
39-
assert!(implementations.contains_key(&"myfunc__real".into()));
37+
assert!(implementations.contains_key("myfunc__int"));
38+
assert!(implementations.contains_key("myfunc__dint"));
39+
assert!(implementations.contains_key("myfunc__real"));
4040
assert_eq!(3, implementations.values().count()); //make sure BYTE-implementation was not added by the annotator
4141

4242
//The pous are added to the index
@@ -500,12 +500,12 @@ fn builtin_generic_functions_do_not_get_specialized_calls() {
500500
let (annotations, _) = TypeAnnotator::visit_unit(&index, &unit);
501501
//The implementations are not added to the index
502502
let implementations = annotations.new_index.get_implementations();
503-
assert!(!implementations.contains_key(&"adr__int".into()));
504-
assert!(!implementations.contains_key(&"adr__dint".into()));
505-
assert!(!implementations.contains_key(&"adr__real".into()));
506-
assert!(!implementations.contains_key(&"ref__int".into()));
507-
assert!(!implementations.contains_key(&"ref__dint".into()));
508-
assert!(!implementations.contains_key(&"ref__real".into()));
503+
assert!(!implementations.contains_key("adr__int"));
504+
assert!(!implementations.contains_key("adr__dint"));
505+
assert!(!implementations.contains_key("adr__real"));
506+
assert!(!implementations.contains_key("ref__int"));
507+
assert!(!implementations.contains_key("ref__dint"));
508+
assert!(!implementations.contains_key("ref__real"));
509509

510510
//The pous are not added to the index
511511
let pous = annotations.new_index.get_pou_types();
@@ -855,7 +855,7 @@ fn resolved_generic_any_real_call_with_ints_added_to_index() {
855855
let (annotations, _) = TypeAnnotator::visit_unit(&index, &unit);
856856
// The implementations are added to the index
857857
let implementations = annotations.new_index.get_implementations();
858-
assert!(implementations.contains_key(&"myfunc__lreal".into()));
858+
assert!(implementations.contains_key("myfunc__lreal"));
859859
assert_eq!(1, implementations.values().count()); //make sure REAL-implementation was not added by the annotator
860860

861861
//The pous are added to the index

src/validation/tests/duplicates_validation_test.rs

+151-3
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,16 @@
1+
use insta::assert_snapshot;
2+
13
use crate::{
2-
ast::{self, SourceRange, SourceRangeFactory},
3-
diagnostics::Diagnostic,
4+
ast::{self, CompilationUnit, SourceRange, SourceRangeFactory},
5+
diagnostics::{Diagnostic, Diagnostician},
46
index::{visitor, Index},
57
lexer::{self, IdProvider},
68
parser,
7-
test_utils::tests::parse_and_validate,
9+
resolver::TypeAnnotator,
10+
test_utils::tests::{compile_to_string, parse_and_validate},
11+
typesystem,
812
validation::Validator,
13+
SourceCode,
914
};
1015

1116
#[test]
@@ -533,3 +538,146 @@ fn automatically_generated_output_types_in_different_files_dont_cause_duplicatio
533538
let diagnostics = validator.diagnostics();
534539
assert_eq!(diagnostics, vec![]);
535540
}
541+
542+
#[test]
543+
fn duplicate_with_generic() {
544+
// a version of the test-util function that does not import the built-in and std-types
545+
// (or they will cause a duplication issue)
546+
fn do_index(src: &str, id_provider: IdProvider, file_name: &str) -> (Index, CompilationUnit) {
547+
let mut index = Index::default();
548+
let (mut unit, ..) = parser::parse(
549+
lexer::lex_with_ids(src, id_provider.clone(), SourceRangeFactory::internal()),
550+
ast::LinkageType::Internal,
551+
file_name,
552+
);
553+
ast::pre_process(&mut unit, id_provider.clone());
554+
index.import(visitor::visit(&unit, id_provider));
555+
(index, unit)
556+
}
557+
558+
// GIVEN a generic function defined in its own file
559+
let ids = IdProvider::default();
560+
let (index1, unit1) = do_index(
561+
r#"
562+
{external}
563+
FUNCTION foo <T: ANY_INT> : DATE
564+
VAR_INPUT
565+
a : T;
566+
b : T;
567+
c : T;
568+
END_VAR
569+
END_FUNCTION
570+
"#,
571+
ids.clone(),
572+
"file1.st",
573+
);
574+
575+
// AND another file that calls that generic function and implicitely
576+
// create type-specific foo-implementations
577+
let (index2, unit2) = do_index(
578+
r#"
579+
PROGRAM prg1
580+
foo(INT#1, SINT#2, SINT#3);
581+
foo(DINT#1, SINT#2, SINT#3);
582+
foo(INT#1, SINT#2, SINT#3);
583+
foo(INT#1, SINT#2, SINT#3);
584+
END_PROGRAM
585+
"#,
586+
ids.clone(),
587+
"file2.st",
588+
);
589+
590+
// AND another file that calls that generic function and implicitely
591+
// create type-specific foo-implementations
592+
let (index3, unit3) = do_index(
593+
r#"
594+
PROGRAM prg2
595+
foo(INT#1, SINT#2, SINT#3);
596+
foo(DINT#1, SINT#2, SINT#3);
597+
foo(INT#1, SINT#2, SINT#3);
598+
foo(INT#1, SINT#2, SINT#3);
599+
END_PROGRAM
600+
"#,
601+
ids,
602+
"file3.st",
603+
);
604+
// WHEN the index is combined
605+
let mut global_index = Index::default();
606+
for data_type in typesystem::get_builtin_types() {
607+
global_index.register_type(data_type);
608+
}
609+
global_index.import(index1); //import file 1
610+
global_index.import(index2); //import file 2
611+
global_index.import(index3); //import file 3
612+
613+
// AND the resolvers does its job
614+
let (mut annotations1, _) = TypeAnnotator::visit_unit(&global_index, &unit1);
615+
let (mut annotations2, _) = TypeAnnotator::visit_unit(&global_index, &unit2);
616+
let (mut annotations3, _) = TypeAnnotator::visit_unit(&global_index, &unit3);
617+
global_index.import(std::mem::take(&mut annotations1.new_index));
618+
global_index.import(std::mem::take(&mut annotations2.new_index));
619+
global_index.import(std::mem::take(&mut annotations3.new_index));
620+
621+
// THEN the index should contain 5 pous, 2 were dynamically generated by the visitor (foo__INT & foo__DINT)
622+
assert_eq!(
623+
vec!["foo", "prg1", "prg2", "foo__INT", "foo__DINT"],
624+
global_index
625+
.get_pous()
626+
.values()
627+
.map(|it| it.get_name())
628+
.collect::<Vec<_>>()
629+
);
630+
631+
// AND there should be no duplication diagnostics
632+
let mut validator = Validator::new();
633+
validator.perform_global_validation(&global_index);
634+
let diagnostics = validator.diagnostics();
635+
assert_eq!(diagnostics, vec![]);
636+
}
637+
638+
#[test]
639+
fn duplicate_with_generic_ir() {
640+
// GIVEN several files with calls to a generic function
641+
let file1: SourceCode = r"
642+
{external}
643+
FUNCTION foo <T: ANY_INT> : DATE
644+
VAR_INPUT
645+
a : T;
646+
b : T;
647+
c : T;
648+
END_VAR
649+
END_FUNCTION
650+
"
651+
.into();
652+
653+
let file2: SourceCode = r"
654+
PROGRAM prg1
655+
foo(INT#1, SINT#2, SINT#3);
656+
foo(DINT#1, SINT#2, SINT#3);
657+
foo(INT#1, SINT#2, SINT#3);
658+
foo(INT#1, SINT#2, SINT#3);
659+
END_PROGRAM
660+
"
661+
.into();
662+
let file3: SourceCode = r"
663+
PROGRAM prg2
664+
foo(INT#1, SINT#2, SINT#3);
665+
foo(DINT#1, SINT#2, SINT#3);
666+
foo(INT#1, SINT#2, SINT#3);
667+
foo(INT#1, SINT#2, SINT#3);
668+
END_PROGRAM
669+
"
670+
.into();
671+
// WHEN we compile
672+
let ir = compile_to_string(
673+
vec![file1, file2, file3],
674+
vec![],
675+
None,
676+
Diagnostician::default(),
677+
)
678+
.unwrap();
679+
680+
// THEN we expect only 1 declaration per type-specific implementation of the generic function
681+
// although file2 & file3 both discovered them independently
682+
assert_snapshot!(ir);
683+
}

0 commit comments

Comments
 (0)