Improve syntax violations interface

[dekellum]

This makes programmatic use of syntax violations more practical while further centralizing this concern in the parser, which should be a maintenance win. An Rc<Fn> is introduced internally to satisfy borrow rules while improving ergonomics. The presumed minimal additional cost of this is only incurred if the syntax violation feature is used. Since existing public interface is maintained and changes are internal, I believe this meets requirements for backward compatibility, and testing agrees, but reviewers should definitely cover that aspect.

Summary of changes

• New SyntaxViolation enum with descriptions the same as the prior violation static strings. This enables programmatic use of these violations by variant, e.g. ignoring some, warning on others, etc. Enum implementation uses the same macro expansion pattern as was used for ParseError.

• New syntax_violation_callback signature in ParseOptions and Parser.

• Deprecated log_syntax_violation and re-implemented as adapter to new callback, passing the same violation descriptions for backward compatibility.

• Unit tests for old log_syntax_violation compatibility, and new syntax_violation_callback including a tested doc example.

• Includes rustdoc for old and new interfaces including example usage of new callback, as requested in Add examples to ParseOptions #308.

Below items edited in light of discussion:

• Improved ergonomics for ParseOptions and syntax_violation_callback. ParseOptions::parse now takes &self, so ParseOptions can still be used repeatedly. The new syntax_violation_callback is passed Fn by value and can be passed inline in user code.

• Potential breaking changes: ParseOptions is no longer Copy, and ParseOptions::parse takes &self (was self, with Copy).

---

This change is 

[dekellum]

Any chance I could get a review on this? @SimonSapin?

[SimonSapin]

Sorry for the delay. I haven’t looked at all the details yet but one thing I notice is that removing ParseOptions: Copy is a breaking change. We’ve accumulated enough desired breaking changes that I’d like to batch them into a 2.0 version of the url crate at some point, but this is effectively blocked on servo/servo#19825.

[dekellum]

Thanks. I think I understand your point, in that removing Copy (keeping Clone) can, hypothetically, lead to breaking consumer code that happens to rely on implicit copy. Please note however that this did not break or require changing any internal, test, or example usage of ParseOptions. Particularly given that ParseOptions is not commonly used, and was not reusable (in current release, prior to this PR) it seems unlikely that consumer code would be copying it?

The pendulum of concern for breaking changes seems to have swung fairly far from center with Rust libs in general of late. I think I may have just missed that memo.

[SimonSapin]

CC @nox, for opinions about SemVer

[nox]

Definitely a breaking change.

[nox]

You could do this instead:

pub trait SyntaxViolationSink {
    fn syntax_violation(&self, violation: SyntaxViolation);
}

impl SyntaxViolationSink for Fn(&'static str) {
    fn syntax_violation(&self, violation: SyntaxViolation) {
        self(violation.description())
    }
}

And then make the methods be:

pub fn log_syntax_violation(self, new: Option<&'a Fn(&'static str)>) -> Self {
    self.syntax_violation_callback(new)
}

pub fn syntax_violation_sink<F>(mut self, new: Option<&'a S>) -> Self
where
    S: SyntaxViolationSink
{
    …
}

Btw @SimonSapin, why is this an Fn and not FnMut?

[nox]

Oh right silly me, if it were an FnMut it wouldn't be Copy anymore anyway.

[nox]

And in my example of code, there is no reason to not use a trait object in syntax_violation_sink:

pub fn syntax_violation_sink<F>(mut self, new: Option<&'a SyntaxViolationSink>) -> Self {
    …
}

[nox]

And finally, Copy is definitely a breaking change because ParseOptions::parse take self. You wouldn't be able to parse 2 URLs without cloning anymore.

[dekellum]

Thanks @nox, very educational. I tried to convert from Fn to FnMut as part of this. I found it difficult in combination with the builder pattern, where ParseOptions should not mutate. Or do you have a suggestion surviving your own scrutiny? I don't fully follow your thoughts.

Regarding your last point. As part of this PR, I also changed parse to take &self so that it can be reused and because that is the more recommended builder pattern, I believe, made possible by Rc<Fn> replacing the &'a Fn. But your comment also made me realize (confirmed by back-porting my tests, cfe75b9) that Copy made ParseOptions reusable previously, and thus this PR just preserves that. I will fix that in my summary.

Finally, I agree that this is breaking, for hypothetical edge cases, but does it require a major release? RFC 1105 seems to offer pragmatism, though this case isn't specifically addressed:

https://github.com/rust-lang/rfcs/blob/master/text/1105-api-evolution.md

all major changes are breaking, but not all breaking changes are major.

[nox]

The question about FnMut was just me digressing, as you discovered the structure cannot be Copy if it wants to use an FnMut, so it cannot be done.

There is a path to avoid the breaking change entirely, so it can definitely be a minor bump.

As for making parse take &self, that doesn't solve the issue that someone could be passing around ParseOptions by value in their codebase and that breaking change would break their code if they reuse it anywhere afterwards.

[dekellum]

Thanks @nox and @SimonSapin. I did find a path forward, resubmitted as #433, which avoids these breaking changes. It doesn't allow for a simple closure wrapping old log Fn to a new callback, but it wasn't as bad as I originally imagined (and what led me to the Rc approach and removing Copy). I hope with the breaking change issue resolved, you will be able to review any other necessary aspects.