halirutan / Wolfram-Language-IntelliJ-Plugin-Archive

Wolfram Language and Mathematica plugin for IntelliJ IDEA.
https://wlplugin.halirutan.de/
Other
193 stars 26 forks source link

Should Block variables be renamed by global 'Rename'? #119

Closed kubaPod closed 5 years ago

kubaPod commented 5 years ago

When renaming variable $whatever with Shift+F6 everything goes well but Block[{$whatever = 1}, ... will stay intact.

I think both approaches, to and not to rename it, have pros and cons but somehow I feel that renaming it will be more beneficial in future.

That of course means that $whatever inside the Block would need to be replaced as well. Now it is shielded.

Shortly, should expressions that implement dynamic scoping be transparent to 'Rename'?

If so, that applies to Internal`InheritedBlock as well.

szhorvat commented 5 years ago

I'm a bit sceptical about making this change. I think this needs more thought.

Consider

var (* 1 *)
Block[{var (* 2 *)}, 
   var (* 3 *)
]

What was your intention?

  1. If we do a refactoring starting from any of occurrences 1, 2 or 3, all three will get replace?
  2. If we do a refactoring starting from 1, all three get replaced. But if we do a refactoring starting from 2 or 3, only 2 and 3 get replaced?

We should also write down concrete use cases (perferably not just made-up ones, but something that provably exsits in the wild).

Use Case 1

Block is often used for trivial, Module-like localization a lot. I use it like this in IGraph/M. The performance increase compared to Module is measurable because the inside of Block has a (very fast) LibraryLink function. I see this use a lot in WRI-written code too. It's not a problem for as long as these variables only get numerical values (and never any symbolic values passed from the outside). Having the local variable reside in the package-context also increases safety.

Example:

f[x_?NumericQ] := Block[{x}, x+=1; x]

For this use case, the current behaviour is ideal and anything else would break code.

Use Case 2

Then there the use case when Block is used to set temporary values to global symbols. I use this extensively in LTemplate:

https://github.com/szhorvat/LTemplate/blob/master/LTemplate/LTemplateInner.m#L253

Example:

current (* is a global variable *)
main[x_, y_] := 
    Block[{current},
        current = getName[x];
        process[y];
    ]

Here main wants to provide some context about what data is being processed to several sub-functions that it is calling. Not having to pass in all this information to the sub-functions as explicit arguments makes the code much more maintainable. Thus main sets this information temporarily on a global variable, which all the callees can access.

In practice, when LTemplate is processing an expression representing a class interface like

LClass["MyClass", {
    LFunction["myfun", ...]
}]

then if it encounters an error while looking at myfun, it want to be able to report a user-friendly error that mentions which class myfun was localted in (in this case, MyClass). This contextual and chaning information is passed down through termpoarily-modified global variables.

For this use case, global renaming would be useful.

Use Case 3

Implement something like Table:

SetAttributes[table, HoldFirst];
table[expr_, {var_, s_, t_}] :=
 Block[{var},
  Function[var = #; expr] /@ Range[s, t]
 ]

This is quite different from Use Case 2 because var in the Block is the same as var in the function. Handling this correctly as well requires a smarter approach. The var in Block should be the same as the var in the pattern, but not the same as a global var.

I think this illustrates how complex this issue is well.

BTW this exact this is why I did not request that Internal`LocalizedBlock should be handled too. A function like table above would usually use LocalizedBlock. The present behaviour of the IDE with

SetAttributes[table, HoldFirst];
table[expr_, {var_, s_, t_}] :=
 Internal`LocalizedBlock[{var},
  Function[var = #; expr] /@ Range[s, t]
 ]

is actually better than if LocalizedBlock would be handled as a localizing construct (similar to Module/Block).

szhorvat commented 5 years ago

My personal opinion on the matter is that Block should be left as it is, for the following reasons:

I am in favour of maintaining the current behaviour. It's not perfect, but clearly useful.

So,

Should Block variables be renamed by global 'Rename'?

My vote is no.

szhorvat commented 5 years ago

@kubaPod Let me know if I misunderstood what you were asking for.

halirutan commented 5 years ago

I agree with the Szabolcs. From the developer's point of view, implementing something like this would require a lot of changes for a very small gain which doesn't even represent the usual use-case.