Skip to content
This repository was archived by the owner on Apr 1, 2025. It is now read-only.

Conversation

@robrix
Copy link
Contributor

@robrix robrix commented Jul 30, 2019

This PR defines a draft of a scope graph analysis, accumulating values representing the declarations and references in a program. This is imperfect in a number of ways which I will enumerate in a review, below, but I’d like to get it merged in as an initial approach which we can contrast with some improved plans.

Scope graph output for one of the example programs
ScopeGraph
  { unScopeGraph =
      fromList
        [ ( Decl
              { declSymbol = "_x"
              , declLoc =
                  Loc
                    { locPath = "src/Analysis/Eval.hs"
                    , locSpan =
                        Span
                          { spanStart = Pos { posLine = 117 , posCol = 20 }
                          , spanEnd = Pos { posLine = 125 , posCol = 5 }
                          }
                    }
              }
          , fromList
              [ Ref
                  Loc
                    { locPath = "src/Analysis/Eval.hs"
                    , locSpan =
                        Span
                          { spanStart = Pos { posLine = 119 , posCol = 13 }
                          , spanEnd = Pos { posLine = 119 , posCol = 28 }
                          }
                    }
              ]
          )
        , ( Decl
              { declSymbol = "_y"
              , declLoc =
                  Loc
                    { locPath = "src/Analysis/Eval.hs"
                    , locSpan =
                        Span
                          { spanStart = Pos { posLine = 117 , posCol = 20 }
                          , spanEnd = Pos { posLine = 125 , posCol = 5 }
                          }
                    }
              }
          , fromList
              [ Ref
                  Loc
                    { locPath = "src/Analysis/Eval.hs"
                    , locSpan =
                        Span
                          { spanStart = Pos { posLine = 120 , posCol = 13 }
                          , spanEnd = Pos { posLine = 120 , posCol = 28 }
                          }
                    }
              ]
          )
        , ( Decl
              { declSymbol = "mkPoint"
              , declLoc =
                  Loc
                    { locPath = "src/Analysis/Eval.hs"
                    , locSpan =
                        Span
                          { spanStart = Pos { posLine = 117 , posCol = 20 }
                          , spanEnd = Pos { posLine = 125 , posCol = 5 }
                          }
                    }
              }
          , fromList
              [ Ref
                  Loc
                    { locPath = "src/Analysis/Eval.hs"
                    , locSpan =
                        Span
                          { spanStart = Pos { posLine = 122 , posCol = 41 }
                          , spanEnd = Pos { posLine = 122 , posCol = 61 }
                          }
                    }
              ]
          )
        , ( Decl
              { declSymbol = "x"
              , declLoc =
                  Loc
                    { locPath = "src/Analysis/Eval.hs"
                    , locSpan =
                        Span
                          { spanStart = Pos { posLine = 118 , posCol = 66 }
                          , spanEnd = Pos { posLine = 121 , posCol = 7 }
                          }
                    }
              }
          , fromList []
          )
        , ( Decl
              { declSymbol = "y"
              , declLoc =
                  Loc
                    { locPath = "src/Analysis/Eval.hs"
                    , locSpan =
                        Span
                          { spanStart = Pos { posLine = 118 , posCol = 66 }
                          , spanEnd = Pos { posLine = 121 , posCol = 7 }
                          }
                    }
              }
          , fromList []
          )
        ]
  }

Copy link
Contributor Author

@robrix robrix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready for review.

Note that we’re likely to want to compose scope graph computation with the computation of e.g. types, so that we can refine scopes based on type information: e.g. if f only gets called with an X, then methods called on its parameter can be looked up on X.

That plus the shortcomings noted below lead me to believe that a better solution in the long run will be for us to treat scope graph computation as an out-of-band accumulation of a graph which composed freely onto other analyses. My current plan for this is based on the abstract garbage collection described in Abstracting Definitional Interpreters, where a simple root-calculating interpreter with knowledge about the syntax it’s evaluating is composed onto the basic interpreter.

This should yield simpler computations of exhaustive scope graphs, and also allow us to tune the precision of the calculation via our selection of another abstract domain.

for_ fields $ \ (k, v) -> do
addr <- alloc k
assign addr v
pure (Value Abstract (foldMap (valueGraph . snd) fields))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were leaving record fields uninitialized in the abstract analyses, which meant that later lookups would fail.

newtype Ref = Ref Loc
deriving (Eq, Ord, Show)

newtype ScopeGraph = ScopeGraph { unScopeGraph :: Map.Map Decl (Set.Set Ref) }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This model of scope graphs has no recursive structure; references map directly to declarations, and not e.g. through other declarations in a path.

This makes this scope graph strictly less informative than those we’re computing in semantic proper, tho it’s hard to say for sure if that makes it strictly less useful for our purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike ImportGraph, our ScopeGraph abstract domain doesn’t require a semi-concrete representation of values alongside it, because we don’t need to treat any of the primitives specially: ImportGraph needs strings and closures modelled explicitly so that it can notice loads issued within methods like Ruby’s require, but ScopeGraph doesn’t require any special treatment of primitives (other than to construct a scope graph).

where alloc = pure
bind name _ m = do
loc <- ask @Loc
local (Map.insert name loc) m
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Binding remembers (locally) the location for the binding site. Unfortunately, this will generally be the location (and especially the Span) of the Ann node surrounding the Lam or :>>=, making it a little imprecise. This could be improved by adding (optional) locations to these nodes for the bound variables, and since Core is curried, we’d do the right thing for multiple parameters automatically.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 File an issue about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m actually just tackling it in a follow-up PR.

ref <- asks Ref
bindLoc <- asks (Map.lookup addr)
cell <- gets (Map.lookup addr >=> nonEmpty . Set.toList)
maybe (pure Nothing) (foldMapA (pure . Just . mappend (extendBinding addr ref bindLoc))) cell
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we’re modelling scope graphs as an abstract domain, we’re limited to producing them in the few methods which take or return a value, notably deref, assign, and record. In particular, we might prefer to build them in lookupEnv or ... in order to preserve nested structure in the latter, but that would require out-of-band scope graph construction e.g. via a Writer effect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we pull out (foldMapA (pure . Just . mappend (extendBinding addr ref bindLoc))) into a let-bound function with a name? It’s a bit intimidating as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0515d7c.

pure (foldMap snd fields')
_ ... m = pure (Just m)

extendBinding addr ref bindLoc = ScopeGraph (maybe Map.empty (\ bindLoc -> Map.singleton (Decl addr bindLoc) (Set.singleton ref)) bindLoc)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bindLoc might be Nothing if e.g. we’re attempting to read a free variable, or if we’re trying to look a name up outside of its lexical binding scope—indirectly, using records. Thus, record field accesses don’t currently get added to the scope graph, which limits the utility of this approach.

@robrix robrix marked this pull request as ready for review July 30, 2019 13:54
@robrix robrix requested a review from a team July 30, 2019 13:54
@robrix robrix mentioned this pull request Aug 6, 2019
8 tasks
Copy link
Contributor

@patrickt patrickt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small request for clarity, but other than that this is fantastic work!

. runReader (Map.empty @User @Loc)
. runFailWithLoc
. fmap fold
. convergeTerm (Proxy @User) (fix (cacheTerm . eval scopeGraphAnalysis))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so readable 😍

ref <- asks Ref
bindLoc <- asks (Map.lookup addr)
cell <- gets (Map.lookup addr >=> nonEmpty . Set.toList)
maybe (pure Nothing) (foldMapA (pure . Just . mappend (extendBinding addr ref bindLoc))) cell
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we pull out (foldMapA (pure . Just . mappend (extendBinding addr ref bindLoc))) into a let-bound function with a name? It’s a bit intimidating as is.

, Member (State (Heap User ScopeGraph)) sig
)
=> Analysis term User ScopeGraph m
scopeGraphAnalysis = Analysis{..}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m normally an anti-RecordWildCards partisan, but this is a 100% copacetic use thereof.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it’ll do until such time as we replace these with effects.

where alloc = pure
bind name _ m = do
loc <- ask @Loc
local (Map.insert name loc) m
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 File an issue about this?

fields' <- for fields $ \ (k, v) -> do
addr <- alloc k
(k, v) <$ assign addr v
-- FIXME: should records reference types by address instead?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this varies based on language? Not sure, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn’t for the types, but might for values.

@robrix robrix changed the base branch from generalize-analyses-over-the-term-type to master August 6, 2019 18:55
@patrickt patrickt merged commit f04a69a into master Aug 6, 2019
@robrix robrix deleted the scope-graphs branch August 6, 2019 19:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants