Remove ParseOptions::syntax_violation_callback?

[SimonSapin]

It’s non-trivial API surface, it’s not tested, and I’m not sure it’s worth supporting. #433 was the only evidence that anyone was using it. @dekellum, can you say more about what you used this for? If you think it should not be removed, why?

[dekellum]

This feature is as per "validation errors" in the URL spec:

https://url.spec.whatwg.org/#validation-error

A validation error indicates a mismatch between input and valid input. User agents, especially conformance checkers, are encouraged to report them somewhere. A validation error does not mean that the parser terminates. Termination of a parser is always stated explicitly, e.g., through a return statement. It is useful to signal validation errors as error-handling can be non-intuitive, legacy user agents might not implement correct error-handling, and the intent of what is written might be unclear to other developers.

I imagine this was originally implemented in rust-url for completeness with the URL spec, which does not seem to imply this is an optional feature for parser implementations?

Servo might eventually wish to log these as warnings in some (e.g. developer) console output?

When and if someone gets around to (re-)writing a conformance checker (URL, HTML, Atom, etc.) in rust, they will most certainly want this.

For rust-url as a general purpose URL parsing library, certain application domains may want to be less lenient, effectively turning certain validation errors into application errors or to filter URLs, as a security precaution, quality heuristic, or for other reasons.

There is currently some testing, though I agree that its not particularly complete.

The lack of current visible usage seems a limited argument for ripping it out. Is there some indication that keeping the feature is going to cost the project, for example with some imminent and extensive refactoring of the parser?

[SimonSapin]

I imagine this was originally implemented in rust-url for completeness with the URL spec, which does not seem to imply this is an optional feature for parser implementations?

The specification says that implementations are “encouraged to report them somewhere” so doing nothing is a conforming implementation.

effectively turning certain validation errors into application errors

I think that might be non-conforming? “A validation error does not mean that the parser terminates. ”

No parser rewrite is planned, but #463 would be an opportunity to remove stuff.

My question was rather: what motivated you to work on #433?

[dekellum]

I interpret the quote differently: Its "User agents, especially conformance checkers", or in other words applications of the URL parser implementation, that are "encouraged to report them somewhere." But the spec doesn't make a super clear distinction between Parser (producing violation errors) and User-Agent or other application (ignoring or consuming them), so I think I can also understand your interpretation.

Broadly, my motivation for #433 is for programmatic usage along the lines I highlighted for "application domains" above. In one case I have a service that receives URLs and I use rust-url to correctly handle IDNA domains and to resolve relative URLs to a given base. However, I need to log a warning if URLs with internal whitespace or other NonUrlCodePoint are encountered, using my own application messaging to explain that results might not be as expected for these URLs.

[nox]

We didn't remove it in 2.0.