On Tue, Feb 19, 2019 at 1:27 PM Vern Paxson <v...@corelight.com> wrote:
> > Which particular user errors were a concern? > > The main one being when there's an API change that's *not* backward > compatible, and the user doesn't update their scripts to be in conformance > with it as is required. Clearly something we'll in general strive to avoid. That's more of a developer error than a user error -- we can make changes that are not backward compatible regardless of what mechanism we go with, and if we do, that's on us (and we do want to always try to make changes that are backward compatible for at least one release cycle). > > What about changing > > semantics of a parameter though? > > That's indeed trickier. Here's a thought for the example you provide: > > event foo%(is_server: bool%) &deprecated; > event foo%(is_client: bool%); > > This would mean "if the handler is defined using the name is_server as the > parameter, that's the deprecated version". Any other declaration (that's > for a parameter of type bool, of course) would refer to the new version. Now we are back to matching parameters on name, so again feels more natural to me to instead just support that generally in the first place. Take another concrete example we've done in the past: change parameter order. event foo(a: bool, b: string); Changes to: event foo(b: string, a: bool); If we generally support matching params by name, then all existing event handlers continue working, no user action needed. Otherwise we need to &deprecate and force users to do work to update their code. > By the way, I didn't follow whether in Vlad's example, the semantics of > the parameter changed, or if his fix was just to give it the correct name > to match its actual semantics. For the latter, existing handlers are > presumably flat-out broken, and there's no benefit in trying to continue > to support them. In Vlad's example, the fix was to make "is_server" actually mean "is server", but people already wrote code based on the real meaning being "is client", so we broke code. (It's a useful example both to learn from and consider how we might do things differently if we had better deprecation procedures/mechanisms). > One concern I have with leaning on is-the-name-a-match is not-too-implausible > user coding along the lines of: > > event foo(is_server_orig: bool) > { > ... > local is_server = my_twisty_logic(is_server_orig, x, y, z); > ... > } > > i.e., the parameter being renamed by the user anyway because they want to > use the original name for a local variable. These users will be bitten > by changes in parameter semantics regardless of which approach we use; > name-based matching isn't a panacea. They wouldn't be bitten if we just don't ever change parameter semantics like that, but use a different deprecation process like I mentioned. E.g. we start with: event foo(is_server: bool); They use: event foo(is_server_orig: bool); We change to: event foo(is_server: bool &deprecated, is_client: bool); Now they get a deprecation warning because we still fall back to matching params by order+type if there's no valid name mappings. The case where they *could* get silently bitten by changes in parameter semantics is if we are actually swapping parameter order, but they happen to swap params of the same type and the user defined custom parameter names. The fix there is either we just don't ever do such ambiguous parameter order swapping or we say that the user is opting into that potential pain for the purely cosmetic reason of custom param names. (Or we rely purely on name-based param matching without falling back to traditional order+type matching. I added the fallback just because it seems generally worthwhile to still support that if people *really* want that, or if they are using custom names already). > > The other problem is that if a user is skipping a version, they may > > end up with a handler that treats "is_server" in the original way, but > > the meaning has been changed and we only match events based on type + > > number of parameters. With name-based parameter matching, we can > > catch this scenario as well. > > With the tweak I outline above, this would only bite them once the &deprecated > version is removed. That could span several releases, depending on the > particular situation. I don't have a lot of sympathy for users who upgrade > across a number of releases, for which the release notes flag backward > compatibility issues, and they don't take heed to address those. I don't sympathize much either, but still though -- if we've got a solution that additionally makes this a non-issue, then that's a Good. > I find an approach that changes the fundamental type-checking used for > event handlers to be more heavy-handed than one that provides control to > the developer to explicitly decide when they've made a change for which > it makes sense to support a deprecated version. There's two points here that I don't think were necessarily related, so taking them separately: * My proposed patch has a more fundamental change to how event handler parameters get type-checked. True, but not sure that is implicitly a Bad. It's maybe a Good if it fits the requirements better (and I still think it does :)) * The two proposals (yours vs. mine) have a difference in the amount of control they grant a developer. Need some clarification here. In my proposal, the dev has just as much control as they always had, plus they get to choose how to deprecate individual event parameters. So that's maybe more control than deprecating entire event signatures? Or what specifically was the difference in developer control that's a concern? Generally I was thinking the mission statement should first emphasize "don't annoy users", then "give developers enough options to make decisions that will annoy users the least". > If Zeek had originally used name-based parameters, then I'd be fine with > this being the solution. However, switching from positional to name-based > strikes me as a conceptually deep change for a relatively modest problem. Slight disagreement on scale of problem -- having solid deprecation procedures/mechanisms is something that's desirable for any software expecting a large user-base. Optimizing for least time-wastage and deprecation periods that give users flexibility in choosing when to address problems is a Good. - Jon _______________________________________________ zeek-dev mailing list zeek-dev@zeek.org http://mailman.icsi.berkeley.edu/mailman/listinfo/zeek-dev