{"action":"created","issue":{"url":"https://api.github.com/repos/dolphinsmalltalk/Dolphin/issues/259","repository_url":"https://api.github.com/repos/dolphinsmalltalk/Dolphin","labels_url":"https://api.github.com/repos/dolphinsmalltalk/Dolphin/issues/259/labels{/name}","comments_url":"https://api.github.com/repos/dolphinsmalltalk/Dolphin/issues/259/comments","events_url":"https://api.github.com/repos/dolphinsmalltalk/Dolphin/issues/259/events","html_url":"https://github.com/dolphinsmalltalk/Dolphin/pull/259","id":189565261,"number":259,"title":"Changing the superclass and instanceVariables of a class in a single operation can produce apparent duplicate or missing instance variables","user":{"login":"daniels220","id":1410866,"avatar_url":"https://avatars.githubusercontent.com/u/1410866?v=3","gravatar_id":"","url":"https://api.github.com/users/daniels220","html_url":"https://github.com/daniels220","followers_url":"https://api.github.com/users/daniels220/followers","following_url":"https://api.github.com/users/daniels220/following{/other_user}","gists_url":"https://api.github.com/users/daniels220/gists{/gist_id}","starred_url":"https://api.github.com/users/daniels220/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/daniels220/subscriptions","organizations_url":"https://api.github.com/users/daniels220/orgs","repos_url":"https://api.github.com/users/daniels220/repos","events_url":"https://api.github.com/users/daniels220/events{/privacy}","received_events_url":"https://api.github.com/users/daniels220/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2016-11-16T02:25:55Z","updated_at":"2016-11-18T06:10:42Z","closed_at":"2016-11-16T21:44:43Z","pull_request":{"url":"https://api.github.com/repos/dolphinsmalltalk/Dolphin/pulls/259","html_url":"https://github.com/dolphinsmalltalk/Dolphin/pull/259","diff_url":"https://github.com/dolphinsmalltalk/Dolphin/pull/259.diff","patch_url":"https://github.com/dolphinsmalltalk/Dolphin/pull/259.patch"},"body":"For example:\r\n\r\n```smalltalk\r\n\"Given this state:\"\r\nObject subclass: #ClassA\r\n    instanceVariableNames: 'var1'\r\n    classVariableNames: ''\r\n    poolDictionaries: ''.\r\nObject subclass: #ClassB\r\n    instanceVariableNames: 'var1 var2'\r\n    classVariableNames: ''\r\n    poolDictionaries: ''.\r\n\"Then, presumably later:\"\r\nClassA subclass: #ClassB\r\n    instanceVariableNames: 'var2'\r\n    classVariableNames: ''\r\n    poolDictionaries: ''.\r\n```\r\n\r\nThe mutation can be performed in-situ, and #mutateInSitu only checks if the superclass has changed—presumably assuming that an instance variable change would require a full mutation, which it usually would, but not in this one unique case.\r\n\r\nSo, the current implementation does not set the instance variables of `ClassB`, and both classes end up declaring `var1`. `ClassB`'s `#instSize` is correct (2), but any new methods referencing `var2` will probably get a BoundsError when run, because the Compiler thinks `var2` is at index 3, instead of the correct 2.\r\n\r\nThis also happens in reverse—given `ClassB` is already a subclass of `ClassA`, evaluating `Object subclass: #ClassB instanceVariableNames: 'var1 var2'` ...etc. leaves ClassB with an `#instSize` of 2, but thinking it only has `var2`, so methods which reference `var1` will presumably not compile. (In both cases, no recompilation is triggered, so existing methods will continue to work correctly until recompiled.)\r\n\r\nOne note: I added `self recompilationRequired: true` to match the superclassIsBeingChanged case, but I'm not sure why recompilation would ever be required when mutating in-situ—doesn't the requirement that #allInstVarNames remains unchanged automatically mean that all existing methods are valid?"},"comment":{"url":"https://api.github.com/repos/dolphinsmalltalk/Dolphin/issues/comments/261456645","html_url":"https://github.com/dolphinsmalltalk/Dolphin/pull/259#issuecomment-261456645","issue_url":"https://api.github.com/repos/dolphinsmalltalk/Dolphin/issues/259","id":261456645,"user":{"login":"daniels220","id":1410866,"avatar_url":"https://avatars.githubusercontent.com/u/1410866?v=3","gravatar_id":"","url":"https://api.github.com/users/daniels220","html_url":"https://github.com/daniels220","followers_url":"https://api.github.com/users/daniels220/followers","following_url":"https://api.github.com/users/daniels220/following{/other_user}","gists_url":"https://api.github.com/users/daniels220/gists{/gist_id}","starred_url":"https://api.github.com/users/daniels220/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/daniels220/subscriptions","organizations_url":"https://api.github.com/users/daniels220/orgs","repos_url":"https://api.github.com/users/daniels220/repos","events_url":"https://api.github.com/users/daniels220/events{/privacy}","received_events_url":"https://api.github.com/users/daniels220/received_events","type":"User","site_admin":false},"created_at":"2016-11-18T06:10:42Z","updated_at":"2016-11-18T06:10:42Z","body":"Riiiight, that makes sense why recompilation would be required when changing superclass. In turn I realize it's not possible to reach this point with inst-var changes and no superclass change—by definition this means the overall inst var layout _would_ change and we would not be doing the mutation in-situ. So, the second `self recompilationRequired: true` would appear to be entirely redundant—does this sound right?\r\n\r\nAlso, re: the comment—d'oh! In general, would you like me to fix issues like this, or does your merging the PR mean either you plan to do it, or don't think it's that big of a deal? (I'm happy to fix it, and I guess if you don't plan to do it, I probably will—the comments in Dolphin are usually both accurate and actually helpful, and I'd hate to knowingly introduce an inaccuracy...)"}}
{"actor":{"display_login":"daniels220"}}