This is an archive of the discontinued LLVM Phabricator instance.

[llgo] Introduce llgoi, a REPL for Go
ClosedPublic

Authored by pcc on Jan 13 2015, 12:50 PM.

Details

Summary

llgoi is a Go REPL based on llgo irgen and the LLVM JIT. It supports
expressions, statements, most declarations and imports, including binary
imports from the standard library and source imports from $GOPATH.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc updated this revision to Diff 18109.Jan 13 2015, 12:50 PM
pcc retitled this revision from to [llgo] Introduce llgoi, a REPL for Go.
pcc updated this object.
pcc edited the test plan for this revision. (Show Details)
pcc added a reviewer: axw.
pcc added a subscriber: Unknown Object (MLST).
axw edited edge metadata.Jan 13 2015, 9:20 PM

This is great, thank you!

I wonder if we should be leaning a little more on go/parser, so we can identify and error on labeled statements, for example. I don't think we need to worry about that just now, though.

cmd/llgoi/llgoi.go
96 ↗(On Diff #18109)

This should only apply to on-the-fly source packages, not ones on-disk. Perhaps add another flag to loadSourcePackage which determines whether or not to augment.

126 ↗(On Diff #18109)

lowercase, for consistency

130 ↗(On Diff #18109)

Can we please follow https://github.com/golang/go/wiki/CodeReviewComments#named-result-parameters, and not named results unless it's helpful documentation-wise? I'd prefer no naked returns either. "return nil, err" makes it immediately obvious what all the result values are.

282 ↗(On Diff #18109)

fmt.Fprintf(&code, "package %s", pkgname)

288 ↗(On Diff #18109)

fmt.Fprintf(&code, "import %q\n", pkg.Path())

312 ↗(On Diff #18109)

Should this error if len(assigns)>0 and there are no new variables?

327 ↗(On Diff #18109)

fmt.Fprintf(&code, " = %s\n\n", l.line)

356 ↗(On Diff #18109)

fmt.Fprintf(&code, "func init() {\n\t%s}", l.line)
(is the whitespace doing anything useful? I guess if we want to log the program source)

476 ↗(On Diff #18109)

Should we be making in.imports a map, to better handle duplicate imports? If you do that now, llgoi panics.

509 ↗(On Diff #18109)

This would be a little neater using bufio.Scanner. I guess eventually we'd want to use GNU readline, or golang.org/x/crypto/ssh/terminal, or ...

pcc updated this revision to Diff 18141.Jan 13 2015, 11:50 PM
pcc edited edge metadata.

Address comments

pcc added a comment.Jan 13 2015, 11:50 PM

I wonder if we should be leaning a little more on go/parser, so we can identify and error on labeled statements, for example. I don't think we need to worry about that just now, though.

I agree that the diagnostics could be improved in many cases. Ideally I would like llgoi to be able to do all parsing/type checking of the user input up front, and treat any errors from parsing/type checking the final package as bugs, but I'm not entirely sure how best this should be achieved.

cmd/llgoi/llgoi.go
96 ↗(On Diff #18109)

Ah, yes, fixed. It seemed neater for loadSourcePackage to take a copts instead.

126 ↗(On Diff #18109)

Done.

130 ↗(On Diff #18109)

Done.

282 ↗(On Diff #18109)

Done.

288 ↗(On Diff #18109)

Done.

312 ↗(On Diff #18109)

Yes it should; I felt that this was more a QOI issue so I left it out of the initial version.

327 ↗(On Diff #18109)

Done

356 ↗(On Diff #18109)

Done.

Yes, the whitespace is semantically insignificant (except for semicolon insertion at newlines), but it is useful if we want to look at the source.

476 ↗(On Diff #18109)

We should be handling a number of things of that nature better. As one other example, if you declare a variable with the same name as an import, llgoi gets confused and refuses to interpret any more lines. I figured this would be good enough for the initial version though.

509 ↗(On Diff #18109)

I guess eventually we'd want to use GNU readline, or golang.org/x/crypto/ssh/terminal, or ...

Yes, or something like that. I think it would be very cool if this could do tab completion, but I couldn't find a suitable package (not to mention the details of actually computing the completions, which doesn't seem easy with the current go/types package).

axw accepted this revision.Jan 13 2015, 11:56 PM
axw edited edge metadata.

LGTM.

Will you be announcing this on golang-nuts? I think you'll make a bunch of people happy, and I can easily see it attracting more help.

This revision is now accepted and ready to land.Jan 13 2015, 11:56 PM
pcc added a comment.Jan 14 2015, 8:14 PM

I'll announce it soon. I would like an upstream change to land first, specifically https://go-review.googlesource.com/#/c/2137/ , as it fixes certain bugs that occur when importing source packages.

This revision was automatically updated to reflect the committed changes.