add first batch of visibilty rules and simple test #1745

michaellilltokiwa · 2023-07-18T12:48:33Z

All the weird/tricky cases regarding visibility seem to be related to inheritance.
Examples:

lets say we inherit from feature in another file that inherits from feature in current file.
should the inherited private features be visible?
We have this feature and return its supertype Any somewhere. Currently calling as_string results in mod even though the redefinition feature might not even be visible.

private mod : Any is
  private redef as_string is
   "mod"

what to do with cases like this?:

module feat : Sequence i_am_private is

I'm all but certain to have thought of all possible cases...

Reusing names for fields (see commented out say b.get_field) is not yet working. crashes:

error 2: java.lang.Error: require-condition3 failed: dev.flang.be.interpreter.Layout:offset:246
	at dev.flang.util.ANY.require(ANY.java:113)
	at dev.flang.be.interpreter.Layout.offset(Layout.java:246)

error 1: java.lang.Error: Value.setField for 'b.field' called on class UNIT (class dev.flang.fuir.analysis.dfa.Value$2), expected class dev.flang.fuir.analysis.dfa.Instance

src/dev/flang/ast/AbstractType.java

src/dev/flang/parser/Parser.java

src/dev/flang/ast/AstErrors.java

src/dev/flang/fe/Module.java

src/dev/flang/fe/SourceModule.java

fridis

Very nice. Particularly like that you did a big effort to check many special cases of visibility.

Just some minor comments suggestions from me.

src/dev/flang/ast/AbstractType.java

fridis · 2023-07-24T07:05:56Z

src/dev/flang/ast/AbstractType.java

+  private Set<AbstractType> usedTypes()
+  {
+    var result = new TreeSet<AbstractType>();
+    usedTypes(result);
+    return result;
+  }


generally, I prefer traversing on demand. There is a method applyToGenericsAndOuter that visits all used types (which is maybe too much), but you could add a variant applyToXYZ.
Here, I think all you need to check is the direct type parameters (see comments below), which are directly available with `generics()´.

I now added an abstract method abstract void usedFeatures(Set<AbstractFeature> s) to AbstractType which collects all the features the type uses/needs into a set. These can then be checked for sufficient visibility.

src/dev/flang/ast/AbstractType.java

src/dev/flang/ast/Visi.java

src/dev/flang/fe/Module.java

src/dev/flang/fe/SourceModule.java

michaellilltokiwa · 2023-07-28T09:26:40Z

#1776 should be merge first

visibility of feature of call is now compared to features visibility having the pre-condition.

print which used type/generic is not visible enough.

fridis

Looks good.

Just some minor restructuring requests and a NYI that can be removed.

fridis · 2023-08-04T09:53:17Z

src/dev/flang/fe/SourceModule.java

@@ -1023,7 +1029,10 @@ public List<FeatureAndOuter> lookup(AbstractFeature outer, String name, Stmnt us
              }
          }

-        for (var e : fs.entrySet())
+        for (var e : fs.entrySet()


Can you remove the NYI-Comment in line 1011: NYI: AND curOuter loaded by this module, which is true since curOuter instanceof Feature was checked.

fridis · 2023-08-04T09:56:41Z

src/dev/flang/fe/SourceModule.java

+                       .filter(fn -> use == null || featureVisible(use.pos()._sourceFile, fn.getValue()))
+                       .collect(List.collector()))
          {
            var v = e.getValue();
            if (!v.isField() || !foundFieldInScope)


I find the code pattern a bit strange here, basically you do

for (var e : fs.filter(x -> cond1(x))) { if (cond2(e)) { result.add(new ..); ] }

which is equivalent to

for (var e : fs) { if (cond1(e) && cond2(e)) { result.add(new ..); ] }

fridis · 2023-08-04T10:00:46Z

src/dev/flang/fe/SourceModule.java

+        var fs = lookup(outer, name, null, traverseOuter)
+                   .stream()
+                   .filter(fo -> typeVisible(pos._sourceFile, fo._feature))
+                   .collect(Collectors.toList());
        for (var fo : fs)
          {
            var f = fo._feature;


Same here, maybe add an if (typeVisible(...)) in the loop instead of using the stream.

fridis · 2023-08-04T10:05:53Z

src/dev/flang/ast/AbstractType.java

+   *
+   * @return this type and any of its generics that have more restrictive visibility than `v`.
+   */
+  public Set<AbstractFeature> moreRestrictiveVisibility(Visi v)


This is ok for now, but think it might be better to use a visitor similar to applyToGenericsAndOuter eventually.

fridis · 2023-08-04T10:08:17Z

src/dev/flang/ast/ResolvedNormalType.java

+   */
+  protected void usedFeatures(Set<AbstractFeature> s)
+  {
+    var f = featureOfType();


This currently does not touch the outer features. This means that for a type like (x T).y U the visibility of x and T will be ignored, which is probably wrong.

But this can be fixed later.

michaellilltokiwa mentioned this pull request Jul 18, 2023

Visi parser 2 #1715

Closed

michaellilltokiwa force-pushed the visibility_rules branch from 8748d84 to 4b1e1b2 Compare July 19, 2023 14:42

maxteufel reviewed Jul 20, 2023

View reviewed changes

michaellilltokiwa marked this pull request as ready for review July 20, 2023 13:32

michaellilltokiwa requested a review from fridis July 20, 2023 13:32

fridis requested changes Jul 24, 2023

View reviewed changes

michaellilltokiwa force-pushed the visibility_rules branch from 3708458 to 92af122 Compare July 24, 2023 13:12

michaellilltokiwa added a commit to michaellilltokiwa/fuzion that referenced this pull request Jul 28, 2023

implement review comments of PR tokiwa-software#1745

81c956f

michaellilltokiwa marked this pull request as draft July 28, 2023 09:26

michaellilltokiwa added a commit to michaellilltokiwa/fuzion that referenced this pull request Jul 28, 2023

implement review comments of PR tokiwa-software#1745

2ebfb74

michaellilltokiwa force-pushed the visibility_rules branch from 81c956f to 2ebfb74 Compare July 28, 2023 09:34

michaellilltokiwa added 8 commits July 31, 2023 11:29

add first batch of visibilty rules and simple test

f51c5d8

checkPreconditionVisibility, fix logic

64b4646

visibility of feature of call is now compared to features visibility having the pre-condition.

improve type visibility errors.

2b30ed3

print which used type/generic is not visible enough.

move visibility errors to AstErrors

a9c1275

visibility, tests: add negative tests

2eb28f7

add comment to checkOuterVisibility

bcc5d77

visibility, improve error messages

3079f79

visibility, add negative test inheriting invisible

cfab4ee

michaellilltokiwa added a commit to michaellilltokiwa/fuzion that referenced this pull request Jul 31, 2023

implement review comments of PR tokiwa-software#1745

a0488fd

michaellilltokiwa force-pushed the visibility_rules branch from 2ebfb74 to a0488fd Compare July 31, 2023 09:31

implement review comments of PR tokiwa-software#1745

265a36c

michaellilltokiwa force-pushed the visibility_rules branch from a0488fd to 265a36c Compare July 31, 2023 09:50

michaellilltokiwa marked this pull request as ready for review August 1, 2023 08:08

michaellilltokiwa requested a review from fridis August 1, 2023 08:08

michaellilltokiwa added 2 commits August 3, 2023 14:54

Merge remote-tracking branch 'origin/main' into visibility_rules

041a6e0

fix merge incongruence

4268c9f

fridis requested changes Aug 4, 2023

View reviewed changes

visibility: restructure code as suggested in review

6085c4a

fridis approved these changes Aug 4, 2023

View reviewed changes

fridis merged commit 8b28373 into tokiwa-software:main Aug 4, 2023
4 checks passed

michaellilltokiwa deleted the visibility_rules branch October 11, 2023 09:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add first batch of visibilty rules and simple test #1745

add first batch of visibilty rules and simple test #1745

michaellilltokiwa commented Jul 18, 2023 •

edited

fridis left a comment

fridis Jul 24, 2023

michaellilltokiwa Jul 28, 2023

michaellilltokiwa commented Jul 28, 2023

fridis left a comment

fridis Aug 4, 2023

fridis Aug 4, 2023

fridis Aug 4, 2023

fridis Aug 4, 2023

fridis Aug 4, 2023

add first batch of visibilty rules and simple test #1745

add first batch of visibilty rules and simple test #1745

Conversation

michaellilltokiwa commented Jul 18, 2023 • edited

fridis left a comment

Choose a reason for hiding this comment

fridis Jul 24, 2023

Choose a reason for hiding this comment

michaellilltokiwa Jul 28, 2023

Choose a reason for hiding this comment

michaellilltokiwa commented Jul 28, 2023

fridis left a comment

Choose a reason for hiding this comment

fridis Aug 4, 2023

Choose a reason for hiding this comment

fridis Aug 4, 2023

Choose a reason for hiding this comment

fridis Aug 4, 2023

Choose a reason for hiding this comment

fridis Aug 4, 2023

Choose a reason for hiding this comment

fridis Aug 4, 2023

Choose a reason for hiding this comment

michaellilltokiwa commented Jul 18, 2023 •

edited