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

Shadow/Undeclared variables #159

Open
jecisc opened this issue Jan 6, 2016 · 22 comments
Open

Shadow/Undeclared variables #159

jecisc opened this issue Jan 6, 2016 · 22 comments

Comments

@jecisc
Copy link
Member

jecisc commented Jan 6, 2016

I think that it would be interesting to explain what is a shadow and an undeclared variable somewhere in UPBE because when I search for information about it some month ago I had a lot of trouble to find information.

@kilon
Copy link
Contributor

kilon commented Jan 6, 2016

no idea what those are and I suspect they are not beginner orientated content either.

@jecisc
Copy link
Member Author

jecisc commented Jan 6, 2016

An instance variable is shadowed if a temporary variable have the same name.
An instance variable (or Class) is Undeclared if the variable (Class) is deleted but still use.

We can see that a lot when we load some code in Jenkins or with a Transcript open but there is almost no documentation about it.

@kilon
Copy link
Contributor

kilon commented Jan 6, 2016

sounds like disaster coding to me... i rather advice people not to use same names for anything and keep their code as clear and readable as possible. This type of coding should not be allowed in the first place.

@jecisc
Copy link
Member Author

jecisc commented Jan 6, 2016

This happen a lot.
Look at Moose builds for example.

With hierarchy you don't always remember all the variables. And with configuration you can forget to define a dependency and you get Undeclared class/variables.

You can see it on the console of a lot of builds:
https://ci.inria.fr/moose/job/roassal2/
https://ci.inria.fr/moose/job/moose-6.0/

But if you do not know what is an undeclared/shadow variable/class you don't know what to do.

@kilon
Copy link
Contributor

kilon commented Jan 6, 2016

But for me this is a bug and not a feature, and bugs should not be documented in tutorials they are meant to be removed. First of all you should not be allowed to create conflicts inside the system even if the system has priorities for those conflict., the system and the language must be clear or else we end up like C++.

The same should apply when you remove a variable, but I dont see that as a problem so much because in that case like any language there should be an error saying that the variable does not exist.

The first is a bug, the second should be a regular error.

In any case thats my opinion, I am not the one to make such decision if this should be documented or not , but the way you describe it my opinion is that it should be not be documented because this way we allow these weaknesses to continue to exist and be accepted as normal and not being reported as what they are really are, bugs and bad designs that must get fixed.

@kilon
Copy link
Contributor

kilon commented Jan 6, 2016

I tested your theory and it appears you are mistaken the system does not allow to define a temporary variable with the same name as an instance variable and it instead complains with the message "Name already defined".

@jecisc
Copy link
Member Author

jecisc commented Jan 6, 2016

This is the case where you do it with a GUI without loading any code. But in headless or if you load some code this happen.

@kilon
Copy link
Contributor

kilon commented Jan 6, 2016

But the opposite is allowed for example you can define a instance variable with the same name as an existing temporary variable, this is definitely a bug.

If in headless you load code and this happens, is a bug, please report it. It should not happen.

@jecisc
Copy link
Member Author

jecisc commented Jan 6, 2016

Since Monticello print it on the Transcript/stdout I think this is the expected behaviour.
If we can't do that, almost all the projects of Pharo-contribution/Moose Jenkins will broke.

@jecisc
Copy link
Member Author

jecisc commented Jan 6, 2016

A variable can also be undefined if you run some code, delete a variable but the program is still running.

@kilon
Copy link
Contributor

kilon commented Jan 6, 2016

Then let them brake, its still a bug, something is not stopping being a bug because if we fix it it brings up more bugs. Its a bug none the less.

"A variable can also be undefined if you run some code, delete a variable but the program is still running."

if the variable is removed then the debugger will complain that it does not exist , no ? why this should be documented ? its common sense. The same will happen with a method, or a class. If you remove something of course the system should complain , we talking here language fundamentals on error reporting.

@jecisc
Copy link
Member Author

jecisc commented Jan 6, 2016

It's just that when people see "xxx is Undeclared" or "xxx is shadowed", if you don't know the problem, you don't know how to fix it.

https://www.youtube.com/watch?v=VrTrQs5sDlA&feature=youtu.be

@kilon
Copy link
Contributor

kilon commented Jan 6, 2016

I hear you loud and clear, but I want to be as clear as possible I dont want workarounds inside this book, I do not want documentation that allows bug and bad designs to exist. I want documnetation that teaches good designs and promotes the strenghs of Pharo. I rather have users complaining about their code braking on the mailing list so someone is motivated to fix these really bad designs than having this accepted as standard behavior.

Again this is my opinion, if you want to add this to this tutorial I wont stop you but I wont accept the pulling request either because I believe is a dangerous mistake. But I wont stop anyone from accepting your pull request either.

@jecisc
Copy link
Member Author

jecisc commented Jan 6, 2016

The goal is not to teach how to do bad design but in the contrary, how to keep the application good. A lot of people will not open a Transcript while loading there code or will not check what is a undeclared variable since it does not break an application most of the time.
Teach how to avoid undeclared or a shadow variable will bring more help that shut the fact that can happen in my opinion.

Maybe UPBE is not the right place for that but I think that this should be documented somewhere accessible because a lot of people don't know what they are and don't care if the application doesn't seems to break.

@Ducasse
Copy link
Member

Ducasse commented Jan 7, 2016

I think that Cyril is 100% correct.
It is important to mention these two points. It is not a question about bad design. This is some consequence on incremental compilation.
I think that having a little paragraph on these concepts is good to have.

@CodeDmitry
Copy link

Wow the fact that this discussion happened really bothers me. Freedom of expression is fundamental in any programming environment and fundamentally outweighs any philosophy you may have. It makes your environment perceived as rigid and makes it hostile to porting concepts from other environments into it. For example, if I want to copy over some concepts from COM, I won't be able to without lots of renaming to satisfy Pharo's obsession with case sensitivity(which is objectively has no reason to have); and if I want to copy over some concepts from functional programming, I won't be able to because once you name something, Pharo goes crazy when you try to shadow it.

Pharo is an amazing environment if you are okay with only using dictionaries and blocks and only use classes/smalltalk object so you can set globals and persist state. But you can't get any real work done through the class browser because it has a very rigid and outdated philosophy on what is right and what is wrong that should be controlled by the user and their workgroup, not by pharo(unless you choose to limit yourself not to be able to register classes from others just because they happen to use shadowing or use lowercase for their class
names and you can't allow any classes to get away with such heresy).

@jecisc
Copy link
Member Author

jecisc commented Mar 16, 2018

@dmitrymakhnin Hi,
I don't have too much time to explain why I think naming convention are important but in the wikipedia pages there is a good start.

https://en.wikipedia.org/wiki/Coding_conventions

Reducing the cost of software maintenance is the most often cited reason for following coding conventions. In their introduction to code conventions for the Java programming language, Sun Microsystems provides the following rationale:
Code conventions are important to programmers for a number of reasons:
- 40%–80% of the lifetime cost of a piece of software goes to maintenance.
- Hardly any software is maintained for its whole life by the original author.
- Code conventions improve the readability of the software, allowing engineers to understand new code more quickly and thoroughly.
- If you ship your source code as a product, you need to make sure it is as well packaged and clean as any other product you create.

@jpfersich
Copy link

jpfersich commented Mar 17, 2018 via email

@bencoman
Copy link
Contributor

Wow the fact that this discussion happened really bothers me. Freedom of expression is fundamental in any programming environment and fundamentally outweighs any philosophy you may have. It makes your environment perceived as rigid and makes it hostile to porting concepts from other environments into it.

Most of this discussion is about documenting the convention not concretely explaining it.

For example, if I want to copy over some concepts from COM, I won't be able to without lots of renaming to satisfy Pharo's obsession with case sensitivity(which is objectively has no reason to have);

Can you give a concrete example of this. These COM classes seem to conform to usual Pharo naming convention... https://msdn.microsoft.com/en-us/library/ms526615(v=exchg.10).aspx

Also I don't believe there is anything concretely preventing you from using all uppercase classes.

Object subclass: #ALLUPPERCASE
instanceVariableNames: ''
classVariableNames: ''
package: 'AAAATest'

ALLUPPERCASE >> test
self inform: 'works okay'

In Playground...
ALLUPPERCASE new test "==> works okay"

Now doing...
Object subclass: #lowercase
instanceVariableNames: ''
classVariableNames: ''
package: 'AAAATest'

lowercase >> test
self inform: 'works okay'

does have a problem parsing...
lowercase new test

requiring...
(Smalltalk at: #lowercase) new test.

and if I want to copy over some concepts from functional programming, I won't be able to because once you name something, Pharo goes crazy when you try to shadow it.

By "goes crazy" are you just referring to Transcript output, or something else?
Perhaps having a pragma to turn off such noise would be useful, as well as helping document such cases in the code.

Pharo is an amazing environment if you are okay with only using dictionaries and blocks and only use classes/smalltalk object so you can set globals and persist state. But you can't get any real work done through the class browser because it has a very rigid and outdated philosophy on what is right and what is wrong that should be controlled by the user and their workgroup, not by pharo(unless you choose to limit yourself not to be able to register classes from others just because they happen to use shadowing or use lowercase for their class names and you can't allow any classes to get away with such heresy).

I believe it would be awfully hard to maintain a program with all-lowercase class names.
But... if you feel strongly about it, present your case on the mail list to have feature via a Setting or Pragma, and more importantly, produce the code (or pay someone) to implement this. After all, they tell us "Pharo is yours".

@bencoman
Copy link
Contributor

sounds like disaster coding to me... i rather advice people not to use same names for anything and keep their code as clear and readable as possible. This type of coding should not be allowed in the first place.

All the way back to Pascal an inner scope variable can override the name of an outer scope variables.
For C... "What if the inner block itself has one variable with the same name?
If an inner block declares a variable with the same name as the variable declared by the outer block, then the visibility of the outer block variable ends at the pint of declaration by inner block
."

I don't think this is something we can realistically get away from allowing. But it is good practice to "report" such cases, thus good to document how to interpret such reports.

I hear you loud and clear, but I want to be as clear as possible I dont want workarounds inside this book, I do not want documentation that allows bug and bad designs to exist.

Its not a work-around. Shadowing is common across many languages. But we should help people understand the reporting. btw, perhaps it would be good to have a tool to "Find Shadow Variables" on the packages context menu, that would pop up list locations which were reported to the Transcript.

@Ducasse
Copy link
Member

Ducasse commented Mar 17, 2018

Hi cyril

We should ask marcus to write a little paragraph on Undeclared. because undeclared have nothing to do with shadowed variables.

Undeclared is you load some code that refer to a class that does not exist or you remove a class that is referenced. The reference is turned into a first class so that we can manipulate after.

Can we please this bug tracker about book issues?
Stef.

@CodeDmitry
Copy link

CodeDmitry commented Apr 9, 2018

@jecisc it is important, but it is up to your workgroup to decide what is important in your workgroup, if you have one, not the vm by default.

@bencoman you can have classes in all capitals, but you can't have classes start with lowercase(without commenting out the part of the validator that does the check, which is easy but can make installation a nightmare with this check forced by default). Likewise, you have to comment out parts of validator to allow methods to start with uppercase(which is a COMlike convention). By going crazy I mean this:

[:m | 
    [:n | Transcript show: n; cr. ] value: m
] value: 99.

works fine but:

[:n | 
    [:n | Transcript show: n; cr. ] value: n
] value: 99.

makes Transcript spam "UndefinedObject>>noMethod(n is shadowed)" and refuse to run. Furthermore, it prohibits you from using any key in Smalltalk globals dictionary inside anonymous blocks that should be oblivious to global scope at compile time, they should welcome shadowing if it means it reduces global dependencies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants