Scoping in TERM does not work. Neither in GM nor in C# (master branch). Example rule:

  ForEach(var, iter, filter, body):
    defines
      Variable var
        of type t
      in filter, body
      where iter has type t

Any reference to var from within filter or body is unresolved.

Corresponding analysed AST for an example:

ForEach(
  "e"{ Def(
         URI(
           Language("Green-Marl")
         , [ ID(NablNsVariable(), "e", Unique("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/0"))
           , Subsequent("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/0")
           , Subsequent("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/0")
           , ID(NablNsProcedure(), "foo", Unique("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/0"))
           , Anonymous("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/0")
           ]
         )
       )
     }
, IterSource(QualifiedCall(VarRef("G"{Use(Result(73))}){(NablProp_graph(), Result(83)), (Type(), Result(75))}, "Nodes", []{(Type(), Result(7))}){(Type(), Result(91))}){(Type(), Result(92))}
, None(){(Type(), Result(4))}
, Block(
    [Assign(VarRef("from"{Use(Result(106))}){(NablProp_graph(), Result(116)), (Type(), Result(108))}, Assign(), VarRef("e"{Use(Result(141))}){(NablProp_graph(), Result(151)), (Type(), Result(143))}, None(){(Type(), Result(4))})]
  )
)

Submitted by Vlad Vergu on 11 July 2013 at 23:39

This was working before. I need to investigate. I assume this is urgent?

Can you please also provide a minimal example in concrete syntax, abstract syntax, and tasks relevant for the resolution?

I’d say mid next week urgent. I’ll need this to work when i get to compilation of actual existent programs. Until then i can leave them unscoped.

Concrete:

	Local foobar(G : Graph, p : N_P<Int>(G)){
	  Foreach(n : G.Nodes){
	    n.p = n.p + 1;
	  }
	}

Abstract:

ForEach(
  "n"{ Def(
         URI(
           Language("Green-Marl")
         , [ID(NablNsVariable(), "n", Unique("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/0")), Anonymous("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/1")]
         )
       )
     }
, IterSource(QualifiedCall(VarRef("G"{Use(Result(25))}){(NablProp_graph(), Result(30)), (Type(), Result(27))}, "Nodes", []{(Type(), Result(40))}){(Type(), Result(52))}){(Type(), Result(53))}
, None(){ (Type(), Result(55))
        , Scope(
            [ ( NablNsVariable()
              , URI(
                  Language("Green-Marl")
                , [Anonymous("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/0"), ID(NablNsProcedure(), "foobar", Unique("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/0")), Anonymous("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/0")]
                )
              )
            ]
          )
        }
, Block(
    [ Assign(
        PropRef(VarRef("n"{Use(Result(81))}){(NablProp_graph(), Result(86)), (Type(), Result(83))}, "p"{Use(Result(115))}){(Type(), Result(117))}
      , Assign()
      , Add(PropRef(VarRef("n"{Use(Result(81))}){(NablProp_graph(), Result(86)), (Type(), Result(83))}, "p"{Use(Result(115))}){(Type(), Result(117))}, IntLit("1"){(Type(), Result(127))}){(Type(), Result(140))}
      , None(){(Type(), Result(35))}
      )
    ]
  ){ Scope(
       [ ( NablNsVariable()
         , URI(
             Language("Green-Marl")
           , [Anonymous("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/1"), ID(NablNsProcedure(), "foobar", Unique("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/0")), Anonymous("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/0")]
           )
         )
       ]
     )
   }
)

Relevant tasks:

		( 62
	  , DisambiguateDefs(Result(61), [], NablNsVariable(), "n")
	  , [61]
	  , []
	  , None()
	  , -1
	  , 0
	  )
	  ( 61
	  , Concat([Result(56), Result(58), Result(60)])
	  , [60, 58, 56]
	  , []
	  , None()
	  , -1
	  , 0
	  )
	  ( 56
	  , ResolveDefs(
	      URI(
	        Language("Green-Marl")
	      , [Anonymous("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/1"), ID(NablNsProcedure(), "foobar", Unique("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/0")), Anonymous("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/0")]
	      )
	    , NablNsVariable()
	    , "n"
	    )
	  , []
	  , Fail()
	  , None()
	  , -1
	  , 0
	  )

@Vlad: Can you please

1. reduce the abstract syntax to def and use site of the variable,
2. reduce the task list to the task mentioned at a use site plus the tasks it depends on?

Thanks.

I will try to fix this on Sunday.

@Guido: I’ve updated the comment above. There are still lots of tasks though.

I assume we are talking the n here. Right?

In the abstract syntax, the use sites are annotated with Use(Result(81)). Task 81 evaluates to a definition, which has the same URI as the variable definition in the ForEach loop. So whatever let’s you think the resolution is broken, it seems the resolution works.

I apologise, the example above was with the scoping rules turned off. I’ve updated the example above. Yes, it’s about the n.

Analysis so far:

1. The definition site is correctly annotated with an URI in a separate anonymous scope.
2. The definition scopes (... in filter, body) are correctly annotated with scope URIs.
3. Tasks 62, 61, 56 try to resolve the right URI.

The bug seems to be caused either by a missing connection between URI in scope, as used by task 56, and the definition site URI in the separate scope, or by a bug in finding such connections.

@Vlad, Gabriël: Is there an index entry that establishes the connection?

I don’t know if this helps, but this is the content of the index related to n:

		Alias(
    URI(
      Language("Green-Marl")
    , [ID(NablNsVariable(), "n", NonUnique()), Anonymous("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/1")]
    )
  , URI(
      Language("Green-Marl")
    , [ID(NablNsVariable(), "n", Unique("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/0")), Anonymous("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/1")]
    )
  )
, Def(
    URI(
      Language("Green-Marl")
    , [ID(NablNsVariable(), "n", Unique("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/0")), Anonymous("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/1")]
    )
  )

Ok, this helps. Can you only show the Def and the Alias? Can you also cut down the task example to tasks 62, 61, 56? That should be enough to understand the problem. The cause seems to be a missing connection between the URI of the Def

URI(
  Language("Green-Marl")
, [ ID(NablNsVariable(), "n", Unique("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/0"))
  , Anonymous("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/1")]
)

and the URI that is used in the resolve task

URI(
  Language("Green-Marl")
, [ Anonymous("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/1")
  , ID(NablNsProcedure(), "foobar", Unique("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/0"))
  , Anonymous("/Users/vladvergu/Documents/workspaces/spoofax-dev/GM_tests/random_bipartite_matching.gm/0")]
)

I probably missed this when switching to a cleaner handling of scopes in the traversal. Will check this tonight and tomorrow night. On which branch are you operating?

I’ve edited my posts as you requested.

I’m operating on the labs branch, but please make changes in a separate branch if you need to; i’m paranoid about loosing track of what, where and how things work. Thank you.

For the Spoofax side of technology i’m using master things.

I found the root cause for the issue. External scopes work like this:

1. When we hit a term that defines a name in an external scope, we mark the external scope by annotating it with the namespace of the name. This happens, when the collection hits the ForEach(...) in the example.
2. When we hit a term that is marked as external scope, we change the annotation to the actual URI of this scope. This happens, when the collection hits the None and Block in the example.

The motivation here is to cover cases, where different terms define names in the same external scope. The solution can handle such cases, without imposing an order on these definitions.

The issue now arises, because at the moment of definition, the external scope is only annotated with a namespace, but not with an URI. That’s why, the connecting alias from name in the external scope to the actually defined name is not created correctly. The fix involves a small runtime change and a medium generator change.

1. The aliasing needs to happen either on the way up (when the external scopes are annotated with URIs), or at the external scope itself. I tend to the first one.
2. The generator needs to generate special rules for external definitions. These rules need to be called from the generic collection.

Starting this now.

I am halfway trough. At the Greenmarl site, I brought back the correct NaBL rule for ForEach. I added a rule in names-fix.str, which should later be generated. At the runtime, I added support for external definitions in the generic collection and fixed the aliases.

The bug is still present, but I cannot work on this before late evening. But maybe Gabriël can figure out why the resolution fails even with the aliases.

I just pushed to existing branches, because I was running out of power.

Sorry for the long comment.

As a heads-up: the names-fix may be breaking something in incrementality and/or dependency tracking. I’ve been getting non-deterministically appearing and disappearing errors on the use sites of the variables defined by the ForEach. I have so far not been able to find a reliable way of reproducing this. I think it is breaking something in incrementality because after a reset & reanalyze everything is back to (1) a correct outcome and (2) always the same outcome.

I’ve added the NaBL rule for the regular For loop, and i’ve fixed the rule for Reduction; duplicating the nabl-external-def-site definitions as per Guido’s fix. I’ve done this to bring the implementation to a consistent state. This was commit 9653d1e.

Commit a4df944 adds SPT tests for this issue. At this stage 6 test fail (45, 46, 49, 51, 54, 55). All four of these tests check for duplicate definition errors when nesting variable definitions within lets. Example:

  Local foobar(G : Graph, p : N_P<Int>(G)) {
    Foreach(n : G.Nodes){
	    Foreach([[n]] : G.Nodes){
        n.p = n.p + 1;
	    }
    }
  }

We expect an error at least on the second definition for n (or two errors, one on each definition). However we get no errors at all. This used to work somewhat (not completely) in commit 6f85f54. In particular test 54 was working and 55 was failing. I think this is a combined bug (something missing in the type of the iterated variable and something in NaBL).

I’ve created a branch fix/nabl-53 to be used for this issue. We’ll merge this back into labs when done.

I’ve left the labs branch with all these changes applied for now. They do not seem to cause blocking problems.

The incrementality issue is surprising. The fix does not create tasks nor does it solve them. However, it stores a Def and an Alias. I assume the dependency tracking does not take care of these in the right way. The reason for this might be in the nature of the URIs, which are special for external definitions.. This is also the root cause for the duplicate definition errors. It reveals a deeper issue with the current way to detect duplicates. Please open a separate issue for this, because this would really be about how to determine duplicates properly.

Fixed the incrementality issue in https://github.com/metaborg/runtime-libraries/commit/826d714f6262bbc38b8cb0320538ddbf77db7de1

Fixed the generator in 738ecb1e18. Definition sites should work now. This might become an issue again for imports into external scopes. I leave this open until that is checked.

Thank you. This works. I’ve merged the fix/nabl-53 branch back into labs and deleted it. We continue this at https://yellowgrass.org/issue/NaBL/60.

The fix applied to the NaBL generator causes a regression in the fix we had for https://yellowgrass.org/issue/NaBL/57