Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Demo prep #685

Merged
merged 101 commits into from
Jul 12, 2022
Merged

Demo prep #685

merged 101 commits into from
Jul 12, 2022

Conversation

robrix
Copy link
Contributor

@robrix robrix commented Jun 9, 2022

This PR fixes up a bunch of stuff for the exception tracing analysis and corresponding demo.

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 🙇🏻

Comment on lines +7 to +10
attr (@this.node) start-line = (start-row @this)
attr (@this.node) start-col = (start-column @this)
attr (@this.node) end-line = (end-row @this)
attr (@this.node) end-col = (end-column @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.

@hendrikvanantwerpen suggested making a single rule for all syntax nodes adding these attributes to the corresponding graph nodes. Unfortunately, not all syntax nodes have a corresponding graph node, whether because we don't intend to map them thus, or because we just haven't got around to writing rules for them yet.

Thus, these attributes are duplicated quite a bit.

Comment on lines +90 to +93
(expression_statement (_) @child) @this
{
let @this.node = @child.node
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

expression_statement wraps a single child syntax node, so there's no point in making a graph node for it.

@@ -67,7 +104,12 @@
attr (@this.node) type = "block"
for child in @children {
edge @this.node -> child.node
attr (@this.node -> child.node) index = (named-child-index child)
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 had been loading statements in arbitrary order 😅


-- | Names of exceptions thrown in the guest language and recorded by this analysis.
--
-- Not to be confused with exceptions thrown in Haskell itself.
newtype Exception = Exception { exceptionName :: Name }
data Exception = Exception { exceptionName :: Name, exceptionLines :: IntSet.IntSet }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exceptions and free variables now include the set of lines they occur on. (Using a set instead of a list is fine because we can always retrieve an ascending list from the set, and we want it uniqued anyway.)

deriving (Eq, Ord, Show)

-- | Sets whose elements are each a variable or an exception.
data ExcSet = ExcSet { freeVariables :: Set.Set Name, exceptions :: Set.Set Exception }
data ExcSet = ExcSet { freeVariables :: Set.Set FreeVariable, exceptions :: Set.Set Exception, strings :: Set.Set Text.Text }
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 include the set of strings as a hack; we raise strings exclusively for the demo, and this is an extremely convenient way to distinguish them.

let sourcePath = replaceExtensions path "py"
-- FIXME: this is comprised of several terrible assumptions.
sourceContents <- Source.fromUTF8 . B.toStrict <$> liftIO (B.readFile sourcePath)
let span = decrSpan (Source.totalSpan sourceContents)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Source.totalSpan is 1-indexed 😩

Comment on lines +197 to +199
chain :: [Term] -> Term
chain [] = Noop
chain (t:ts) = foldl' (:>>) t ts
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 no longer insert spurious no-ops at the beginning of non-empty lists of statements.

Comment on lines +216 to +228
locate :: A.Object -> A.Parser (Graph -> Term) -> A.Parser (Graph -> Term)
locate attrs p = do
span <- span
<$> attrs A..:? fromString "start-line"
<*> attrs A..:? fromString "start-col"
<*> attrs A..:? fromString "end-line"
<*> attrs A..:? fromString "end-col"
t <- p
case span of
Nothing -> pure t
Just s -> pure (Locate s <$> t)
where
span sl sc el ec = Span <$> (Pos <$> sl <*> sc) <*> (Pos <$> el <*> ec)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, at least we only have to write it once.

Comment on lines +233 to +248
analyzeFile
:: (Algebra sig m, MonadIO m)
=> FilePath
-> ( forall term
. Ord term
=> ( forall sig m
. (Has (Env addr) sig m, HasLabelled Store (Store addr val) sig m, Has (Dom val) sig m, Has (Reader Reference) sig m, Has S.Statement sig m)
=> (term -> m val)
-> (term -> m val) )
-> Source.Source
-> File term
-> m b )
-> m b
analyzeFile path analyze = do
(src, file) <- parseToTerm path
analyze eval src file
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 had been more useful before parseToTerm existed, but it's still handy to e.g. conveniently print an exception set.

Comment on lines +250 to +253
parseToTerm :: (Algebra sig m, MonadIO m) => FilePath -> m (Source.Source, File Term)
parseToTerm path = do
parsed <- runThrow @String (parseFile path)
either (liftIO . throwIO . ErrorCall) pure parsed
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main thing this accomplishes is to turn the Either produced on line 252 into an ErrorCall so that we don't have to deal with unwrapping the source and file in the REPL.

@robrix robrix marked this pull request as ready for review June 9, 2022 15:25
@robrix robrix requested a review from a team as a code owner June 9, 2022 15:25
semantic-analysis/src/Analysis/Analysis/Exception.hs Outdated Show resolved Hide resolved
Comment on lines 107 to 124
printExcSet :: Source.Source -> ExcSet -> IO ()
printExcSet src e = for_ (zip [0..] (Source.lines src)) $ \ (i, line) -> do
Text.putStr (keywords (Text.dropWhileEnd (== '\n') (Source.toText line)))
let es = exceptionsForLine i e
fvs = freeVariablesForLine i e
unless (null es && null fvs) $ do
Text.putStr (Text.pack " \ESC[30;1m# ")
Text.putStr (Text.pack "{" <> union
( formatFreeVariables fvs
<> formatExceptions es ) <> Text.pack "}" <> reset)
Text.putStrLn mempty
where
keyword k s = Text.intercalate (Text.pack "\ESC[34;1m" <> k <> reset) (Text.splitOn k s)
keywords = keyword (Text.pack "raise") . keyword (Text.pack "import") . keyword (Text.pack "def") . keyword (Text.pack "pass")
union = Text.intercalate (Text.pack ", ")
formatFreeVariables fvs = map (\ fv -> Text.pack "?" <> formatName (freeVariableName fv)) (Set.toList fvs)
formatExceptions excs = map (Text.pack . show . formatName . exceptionName) (Set.toList excs)
reset = Text.pack "\ESC[0m"
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is gonna be over large data sets, you might want to use a Builder here?

semantic-analysis/src/Analysis/Syntax.hs Show resolved Hide resolved
semantic-analysis/src/Analysis/Syntax.hs Show resolved Hide resolved
rewinfrey
rewinfrey previously approved these changes Jun 29, 2022
Copy link
Member

@rewinfrey rewinfrey left a comment

Choose a reason for hiding this comment

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

👍 @robrix, it's exciting to see the exception tracing demo! I see there are places in the code where future efforts can help refine the approach, but for the demo this looks great to me.

@robrix
Copy link
Contributor Author

robrix commented Jul 11, 2022

@patrickt: for whatever reason I can't reply to #685 (comment) (re: using Builder instead of constructing Text by <>) directly. This is both debug and demo code, and is run over small programs (pretty much by necessity since we don't support most of Python yet), so I'm comfortable leaving it in the direct-but-stupid mode… for now.

Given that this is my response to a few of the above (excellent!) comments, I think it's worth making explicit that we haven't yet decided if this is going to become more than a hacked-up prototype, i.e. whether we're going to put any more engineering effort into it. If we are, this definitely deserves a follow-up PR resolving all of these, but if not it'd be wasted time—except insofar as future prototypes might develop further; but we can resolve these issues at that point.

Lazy evaluation, in other words 😅

@robrix robrix requested a review from a team July 11, 2022 13:57
@robrix
Copy link
Contributor Author

robrix commented Jul 11, 2022

Did the OverloadedStrings thing; re-review would be greatly appreciated! ❤️

@robrix robrix merged commit b0ccf17 into main Jul 12, 2022
@robrix robrix deleted the demo-memo branch July 12, 2022 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants