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

Can Var have an id field? #28

Open
cscherrer opened this issue Feb 28, 2022 · 8 comments · May be fixed by #29
Open

Can Var have an id field? #28

cscherrer opened this issue Feb 28, 2022 · 8 comments · May be fixed by #29

Comments

@cscherrer
Copy link
Contributor

For code rewriting, it can be really useful to have more to distinguish a variable than just its name. For example, we might want to do things like "rename all occurrences of this x to y" or even "convert this AST to SSA style".

This could be very easy if JuliaVariables could be extended so Var has an additional id field. Do you have how difficult that would be to add? If it would be a major rewrite I could try to find other ways of getting to this.

@thautwarm
Copy link
Member

It's possible, as SymRef should be its object identity. We can assign id to Var at

function from_symref(s::SymRef)

the same SymRef creates the same id in Var.

NOTE that the global variables and keyword arguments cannot be refactored by JuliaVariables.

@cscherrer
Copy link
Contributor Author

Ok, I have at least a start on this. I'll PR soon to see what you think.

@cscherrer cscherrer linked a pull request Mar 1, 2022 that will close this issue
@cscherrer
Copy link
Contributor Author

Hmm, maybe unique_id should be the same as name for globals, and we only increment the counter for local variables?

@thautwarm
Copy link
Member

thautwarm commented Mar 2, 2022

Sorry, SymRef is not the object identity of the resolved symbols. We should handle LocalVar and GlobalVar(Symbol).

I think your modification to local_var_to_var might not work for some cases. Same symbols in different scopes might result in different id.

Fortunately, LocalVar.is_mutable or LocalVar.is_shared are references, they can be the real object identity of the symbol. NameResolution.jl recognizes the same LocalVar for their different occurrences.

https://github.com/JuliaStaging/NameResolution.jl/blob/463ad09584ce823514a58bbc1391c6db51035a0b/src/Variable.jl#L2

You can use either is_mutable and is_shared as the reference. I'd suggest such a structure for Var:

struct Var
    name::Symbol
    unique_id:: Union{Ref{Bool}, Nothing}  # nothing when it's a global variable
    is_mutable::Bool
    is_shared::Bool
    is_global::Bool
end

@cscherrer
Copy link
Contributor Author

Ok, I think I understand, but just to be sure... For two LocalVars, are the LocalVar.is_mutables the same (===) exactly when the LocalVar.is_shareds are the same?

@thautwarm
Copy link
Member

@cscherrer Yes.. Sorry for such redundancy, Maybe I should make LocalVar a mutable struct then keeping a LocalVar is sufficent..

@cscherrer
Copy link
Contributor Author

That would make sense, but it's not such a big deal either way. I think I'd refactor it this way if you think you'll be extending it more, since that could be easier to build on. But if it's basically done I'd maybe not worry about it. Up to you of course :)

cscherrer added a commit to cscherrer/JuliaVariables.jl that referenced this issue Mar 17, 2022
@cscherrer
Copy link
Contributor Author

Ok, I've updated the PR.

Also, what do you think of allowing another method passing an id_function? I've prototyped the idea here:
cscherrer#1

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 a pull request may close this issue.

2 participants