Skip to content

starlark: Add Module type #595

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

Merged
merged 3 commits into from
Jun 23, 2025
Merged

Conversation

timothyg-stripe
Copy link
Contributor

@timothyg-stripe timothyg-stripe commented Jun 18, 2025

This PR adds a new Module type and surrounding API:

type Module struct{ … }
func (*Module) Program()     *Program
func (*Module) Globals()     StringDict
func (*Module) Predeclared() StringDict

func (*Program) InitModule(thread *Thread, predeclared StringDict) (*Module, error)

func (*Function) Module() *Module

The existing (*Program).Init function is reimplemented in terms of InitModule. Similarly, fn.Globals() is now the same as fn.Module().Globals().

Specifically, this adds a way to get the predeclared dictionary for a given Function, which makes it easier to write a debugger.

Fixes #594

// variables so far defined in the function's module.
func (fn *Function) Globals() StringDict { return fn.module.makeGlobalDict() }

// Predeclared returns a new StringDict containing the
// predeclared names in the function's module.
func (fn *Function) Predeclared() StringDict {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if it would make more sense to expose the Module type in the API (keeping its fields private), and to add the Predeclared method onto Module? That way it is clear that fn.Module().Predeclared() accesses a value that is shared among all functions of the same module (even if there's today nothing else you can do with the module--though in future there may be).

Also, unlike Globals, Predeclared should just return a reference to the StringDict, not a copy, since the user provided us with it in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should Globals live on this Module type too then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that seems appropriate, though of course we'll have to keep the existing method on Function as a wrapper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, PTAL.

@timothyg-stripe timothyg-stripe changed the title starlark: Add (*Function).Predeclared starlark: Add Module type Jun 23, 2025
@timothyg-stripe timothyg-stripe requested a review from adonovan June 23, 2025 21:45
Copy link
Collaborator

@adonovan adonovan left a comment

Choose a reason for hiding this comment

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

Test looks good; thanks.

@timothyg-stripe timothyg-stripe requested a review from adonovan June 23, 2025 22:29
@timothyg-stripe
Copy link
Contributor Author

fixed all the comments

Copy link
Collaborator

@adonovan adonovan left a comment

Choose a reason for hiding this comment

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

LGTM, thanks again.

@adonovan adonovan merged commit 8bf495b into google:master Jun 23, 2025
13 checks passed
@timothyg-stripe timothyg-stripe deleted the predeclared branch June 23, 2025 22:32
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.

Add (*starlark.Function).Predeclared()
2 participants