I just spent hours trying to diagnose a problem where Rojo had accidentally created a ModuleScript and a Folder with the same name, and any scripts which tried to access the Folder instead accessed the ModuleScript.
I propose that, if a script accesses an instance with a non-unique name like this, in a way that can be detected statically (e.g. workspace.Part, workspace:FindFirstChild(“Part”), etc), two things should occur;
The expression should be highlighted in the script editor as a warning
The instances should be highlighted in Explorer with a warning icon
I propose the second warning location because I work in an external code editor, and so do not see Roblox’s Script Analysis window. Instead, I use Luau LSP in VS Code, which 1) provides its own script analysis so there’s no reason for me to have both Luau LSP’s and Roblox Studio’s script analysis widgets open, and 2) since this was a bug caused by Rojo, Luau LSP could not have caught this error anyway. I naturally defaulted to using the Explorer to try and inspect the instance tree, but it did not help in this situation because, despite instances being sorted alphabetically, they’re also sorted by class type, which meant the two identically named instances did not show up next to each other and so made spotting the error difficult.
ok jokes aside, the real issue is that Roblox’s weak DOM allows two or more instances with the same name. I get why this can be useful for non-scripted areas of the codebase, ie parts or ui elements.
But for scripts, folders and modules it should really enforce a restriction like this on names, maybe with a warning in the explorer?
i remember i felt myself a dumbass on why wouldn’t X work until i realised there’s 2nd instance disrupting what i wanted to do
so annoying. hope this is addressed
im pretty sure Godot does not allow two nodes of same name with their parent being same.
though i can imagine how it just becomes smth like Part (17328)
I don’t like that this encourages developers to rely on the order of instances given that order isn’t documented / explained / guaranteed anywhere and it can change in ways that are hard to observe or predict.
I would swap this line out for a recommendation to use unique names for children that need to be directly acccessed. I can the value in offering an easier fix that silences the warning, though.
Yeah, that’s probably a bit too encouraging. But I do think the warning should point you at :FindFirstChild if you really need to bypass the warning because warnings you can’t bypass are really frustrating (WaitForChild waited too long anyone?)
Ambiguous child access, <path> has multiple children named <name>.
You may access the first one with :FindFirstChild("<name>") but child order is
not guaranteed so using unique names instead is recommended.
I’m planning to implement this as a runtime warning. Do you think that’s incorrect?
I feel like there’s definitely cases where you’d run into this dynamically where a static lint wouldn’t catch it. In fact, some of the cases where I can personally remember running into it were runtime scenarios.
Either would work, but I think it’d be ideal to catch as many cases as possible statically, because static lints are much more immediate and mean I don’t have to run the entire project to catch the error.
I’m not too worried about that because in my experience this is a pretty rare mistake so as long as you get feedback somehow it’s good. I think I’ve only run into it about a dozen times total.
About suppressing it with FindFirstChild - why should people be able to suppress it? If they need to pick out a certain instance from a group of identically named instances, shouldn’t they use GetChildren? I’m struggling to see any case where it’d be useful to rely on the ordering of FFC implicitly.
The API is literally called FindFirstChild, there’s not much we can do to stop people when there’s a prevalent API with a clear contract supporting that.
Ok fair enough, I guess I’m just not happy because it’s pretty bad practice to actually depend on that contract in any serious capacity given that order is completely undefined. Even if not via FFC, I maintain there should be a checked way of getting a child safely, as opposed to either having to unsafely index to get the warning, or having to give up the ambiguity checks.
If I were to propose an API surface, it’d probably be something like a generic :FindChild() method that can also be configured to do things like look for ancestors, descendants, instances of certain class types, etc. That could then replace the forest of methods we have right now for doing all that stuff, allows for the return type to be fully deterministic by explicitly disallowing ambiguity, and maybe - depending on the specific API surface - could solve for some longstanding problems such as wanting to index deep into a structure without having to deal with nils at every step of the way.
Though, for the purposes of this feature request, I think the primary concern is the fact that we just need a way to safely get a child that may not be there without losing the ambiguity checks.
I made this exact mistake yesterday in a code org change. I think there’s value in both a runtime warning and a linting warning for those who don’t rely on the code editor (given RDC’s announcements)
I like this possible warning, but I think it would better to not mention FFC as an option due to the unintentional UB due to the child order not being guaranteed.
Maybe referring to FindFirstChildOfClass makes more sense? Or just suggest renaming, if you know enough about FFC’s implementation you would know when to override this sort of behaviour
Thanks for the active feedback, it’s very much appreciated.
