Improve syntax violations interface and preserve Copy #433

dekellum · 2018-02-05T19:09:51Z

This is an alternative implementation of #430 which avoids introducing an Rc<Fn> and preserves Copy for ParseOptions. Thus, based on my current understanding with the prior review, it is completely backward compatible and with no breaking changes. Its a little less elegant in that, without the Rc<Fn>, I can not simple wrap the deprecated log_syntax_violation Fn with an adapting closure. But by moving the two possible function references into a new enum ViolationFn it is reasonably well contained. The interface for users is effectively the same, so I think the backward compatibility advantage makes this preferable to #430. Updated summary:

Summary of changes

This makes programmatic use of syntax violations more practical while further centralizing this concern in the parser, which should be a maintenance win.

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, providing translated descriptions, 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, passing the same violation descriptions for backward compatibility.
Unit tests for old log_syntax_violation compatibility, and new syntax_violation_callback
Includes rustdoc for old and new interfaces including tested example usage of new callback, as requested in Add examples to ParseOptions #308.

This change is

nox · 2018-02-05T21:50:57Z

Wouldn't it be better to introduce a separate trait rather than use a different Fn? If we ever want to extend it, we should rather not rely on Fn directly.

dekellum · 2018-02-05T22:17:56Z

The extension scenarios I could imagine would be best applied to the introduced SyntaxViolation (methods or tuple additions if needed). My goal is to make this part of the interface easier to use. Unfortunately the builder situation won't allow FnMut, but an alternative new trait would also need to be called with a non-mutable reference, and I believe would only increase the burden to the user, who would now need to implement the trait outside of block context with the options building and parse call? Consider the additions needed for the provided new tests, or the doc example.

dekellum · 2018-02-05T23:04:17Z

Well, per my understanding of your trait example in #430, the following could adapt back to the current Fn(SyntaxViolation) as seen in tests and examples, so it would allow having it both ways:

impl SyntaxViolationSink for Fn(SyntaxViolation) {
    fn syntax_violation(&self, violation: SyntaxViolation) {
        self(violation)
    }
}

But its more interface surface if SyntaxViolationSink is exported, and I think it needs to be for external closure-based usage?

Note that the ViolationFn of this PR doesn't need to be, and isn't exported. Otherwise it should roughly be the same amount of code. I'm not seeing the extension advantage and I doubt there is practical uses of a Sink without the context of the other parse options, the url input str being parsed, and any ParseError?

dekellum · 2018-02-07T19:38:20Z

I hope I've adequately addressed @nox's concern on extensiblity and implementation strategy. @SimonSapin: Could you review the remaining aspects of this, now with the breaking change concern hopefully out of the way?

nox · 2018-02-09T09:39:53Z

tests/unit.rs

-    };
+    let url = Url::options().
+        log_syntax_violation(Some(&|s| violation.set(Some(s.to_owned())))).
+        parse("http:////mozilla.org:42").unwrap();


This is badly indented, the periods shouldn't be at the end of lines, but at the start.

Converted to periods-on-the-left style in all PR changes.

nox · 2018-02-09T09:40:33Z

src/lib.rs

+    /// `syntax_violation_callback` and is implemented as an adaptor for the
+    /// latter, passing the `SyntaxViolation` description. Only the last value
+    /// passed to either method will be used by a parser.
+    #[deprecated]


I'm not sure the method should immediately be made deprecated.

Its not required for my use case. But why not give deprecation warning to those (relatively few) users of this feature to migrate to Fn(SyntaxViolation), calling v.description() themselves if that is all they want? If deprecated in the next minor release (merged), then this could be removed in some future major release, as cleanup.

nox · 2018-02-09T09:41:07Z

src/lib.rs

@@ -186,7 +186,7 @@ impl HeapSizeOf for Url {
 pub struct ParseOptions<'a> {
    base_url: Option<&'a Url>,
    encoding_override: encoding::EncodingOverride,
-    log_syntax_violation: Option<&'a Fn(&'static str)>,
+    violation_fn: ViolationFn<'a>,


Please store an Option<ViolationFn<'a>>and remove theNoOp` variant.

If I did that, it would complicate things. Firstly, I couldn't directly implement the helper methods call and call_if, given the outer type is now Option. Secondly it would complicate those methods even if I could. See the comments around call_if below.

I see. Why not make the methods on Parser instead of ViolationFn? This would let you use Option<T>, right?

OK, for preview, in 95a7b4a I reinstate Option and move the necessary methods back to the Parser. This change would reduce the overall diff for the feature, keeping more status-quo with the parser. However it spreads around the if let Some(vfn) ... and the *_if function can't be used outside of the Parser impl. Input already uses ViolationFn::call. If the *_if variant is ever needed elsewhere this will prove a liability.

I personally still think the current PR is better without this change, but if you prefer 95a7b4a, I will include it in this branch and PR.

nox · 2018-02-09T09:41:41Z

src/lib.rs

+    /// # }
+    /// # run().unwrap();
+    /// ```
+    pub fn syntax_violation_callback<F>(mut self, new: Option<&'a F>) -> Self


Remove F and pass a trait object here too.

Simplified in 6017325. This was legacy from my prior Rc version with pass by value requiring <F>, but I see that it has no remaining value.

nox · 2018-02-09T09:43:15Z

src/parser.rs

+        }
+    }
+
+    pub fn call_if<F>(self, v: SyntaxViolation, test: F)


Please just use if where you currently call call_if.

Violation#call_if is direct replacement for the original Parser#syntax_violation_if. I think this was introduced as and remains a useful optimization and convenience in that it does an early check for NoOp (short-circuit eval of test). Beyond that I have been trying not to change every call by needing to rewrite with if blocks, which would be a much more extensive change of the Parser, and require more scrutiny.

While I could have kept and just modified the original Parser method in place, I think this PR better collects those concerns under ViolationFn. I added comments to these ViolationFn methods to hopefully clear this up. Hope that is sufficient.

nox · 2018-02-09T09:43:29Z

src/parser.rs

@@ -216,11 +263,56 @@ impl<'i> Iterator for Input<'i> {
    }
 }

+/// Wrapper for syntax violation callback functions.
+#[derive(Copy, Clone)]
+pub enum ViolationFn<'a> {


Make this pub(crate).

Changing this to pub(crate) fails with the following errors on the parser module public, but crate private, structs Input and Parser. I suspect that all of these cases would need to be made pub(crate) as well, or module private, but I'm reluctant to add that to this PR, as it expands scope and risk. If it is worth it, consider as a follow-on change?

error[E0446]: private type `parser::ViolationFn<'_>` in public interface --> src/parser.rs:166:5 | 166 | / pub fn with_log(original_input: &'i str, vfn: ViolationFn) -> Self { 167 | | let input = original_input.trim_matches(c0_control_or_space); 168 | | if vfn.is_set() { 169 | | if input.len() < original_input.len() { ... | 176 | | Input { chars: input.chars() } 177 | | } | |_____^ can't leak private type error[E0446]: private type `parser::ViolationFn<'a>` in public interface --> src/parser.rs:319:5 | 319 | pub violation_fn: ViolationFn<'a>, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ can't leak private type ~~~

I guess that's ok, I just wanted to make sure ViolationFn doesn't escape the crate.

dekellum · 2018-02-13T01:21:58Z

There is one remaining review item and resolution option, @nox?

nox · 2018-02-15T08:49:30Z

This looks ok. Please squash the commits accordingly and I'll r=me it.

* New SyntaxViolation enum with description the same as prior strings * Replaced Fn signature in ParseOptions and Parser with ViolationFn wrapper enum for old and new functions * ParseOptions: Copy is preserved

dekellum · 2018-02-15T18:24:30Z

Squashed to 2 commits, where 255df06 has no diff. from 96148cf, the previously reviewed branch head.

nox · 2018-02-16T13:41:37Z

@bors-servo r+

bors-servo · 2018-02-16T13:41:38Z

📌 Commit 255df06 has been approved by nox

bors-servo · 2018-02-16T13:41:40Z

⌛ Testing commit 255df06 with merge 32c2006...

Improve syntax violations interface and preserve Copy This is an alternative implementation of #430 which avoids introducing an `Rc<Fn>` and preserves `Copy` for `ParseOptions`. Thus, based on my current understanding with the prior review, it is completely backward compatible and with no breaking changes. Its a little less elegant in that, without the `Rc<Fn>`, I can not simple wrap the deprecated log_syntax_violation Fn with an adapting closure. But by moving the two possible function references into a new enum `ViolationFn` it is reasonably well contained. The interface for users is effectively the same, so I think the backward compatibility advantage makes this preferable to #430. Updated summary: ### Summary of changes This makes programmatic use of syntax violations more practical while further centralizing this concern in the parser, which should be a maintenance win. * 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, providing translated descriptions, 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, passing the same violation descriptions for backward compatibility. * Unit tests for old `log_syntax_violation` compatibility, and new `syntax_violation_callback` * Includes rustdoc for old and new interfaces including tested example usage of new callback, as requested in #308.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-url/433)

bors-servo · 2018-02-16T13:45:06Z

☀️ Test successful - status-travis
Approved by: nox
Pushing 32c2006 to master...

At least #433 added new APIs

dekellum mentioned this pull request Feb 5, 2018

Improve syntax violations interface #430

Closed

nox suggested changes Feb 9, 2018

View reviewed changes

dekellum added 2 commits February 15, 2018 09:58

Test for existing log_violation_syntax interface

42b82fa

Add syntax_violation_callback, SyntaxViolation enum

255df06

* New SyntaxViolation enum with description the same as prior strings * Replaced Fn signature in ParseOptions and Parser with ViolationFn wrapper enum for old and new functions * ParseOptions: Copy is preserved

dekellum force-pushed the improve-violations-interface-2 branch from 96148cf to 255df06 Compare February 15, 2018 18:08

nox approved these changes Feb 16, 2018

View reviewed changes

bors-servo merged commit 255df06 into servo:master Feb 16, 2018

dekellum mentioned this pull request Feb 16, 2018

Add examples to ParseOptions #308

Closed

SimonSapin added a commit that referenced this pull request Feb 20, 2018

1.7.0

1b84a39

At least #433 added new APIs

SimonSapin mentioned this pull request Oct 17, 2018

Remove ParseOptions::syntax_violation_callback? #462

Closed

This was referenced Jul 13, 2019

Remove the deprecated log_syntax_violation function for 2.0 #509

Closed

Remove the deprecated log_syntax_violation function for 2.0 #511

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve syntax violations interface and preserve Copy #433

Improve syntax violations interface and preserve Copy #433

dekellum commented Feb 5, 2018 •

edited by larsbergstrom

Loading

nox commented Feb 5, 2018

dekellum commented Feb 5, 2018 •

edited

Loading

dekellum commented Feb 5, 2018 •

edited

Loading

dekellum commented Feb 7, 2018

nox Feb 9, 2018

dekellum Feb 9, 2018

nox Feb 9, 2018

dekellum Feb 9, 2018 •

edited

Loading

nox Feb 9, 2018

dekellum Feb 9, 2018

nox Feb 10, 2018

dekellum Feb 10, 2018

nox Feb 9, 2018

dekellum Feb 9, 2018

nox Feb 9, 2018

dekellum Feb 9, 2018

nox Feb 9, 2018

dekellum Feb 9, 2018 •

edited

Loading

nox Feb 10, 2018

dekellum commented Feb 13, 2018

nox commented Feb 15, 2018

dekellum commented Feb 15, 2018

nox commented Feb 16, 2018

bors-servo commented Feb 16, 2018

bors-servo commented Feb 16, 2018

bors-servo commented Feb 16, 2018

Improve syntax violations interface and preserve Copy #433

Improve syntax violations interface and preserve Copy #433

Conversation

dekellum commented Feb 5, 2018 • edited by larsbergstrom Loading

Summary of changes

nox commented Feb 5, 2018

dekellum commented Feb 5, 2018 • edited Loading

dekellum commented Feb 5, 2018 • edited Loading

dekellum commented Feb 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dekellum Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dekellum Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dekellum commented Feb 13, 2018

nox commented Feb 15, 2018

dekellum commented Feb 15, 2018

nox commented Feb 16, 2018

bors-servo commented Feb 16, 2018

bors-servo commented Feb 16, 2018

bors-servo commented Feb 16, 2018

dekellum commented Feb 5, 2018 •

edited by larsbergstrom

Loading

dekellum commented Feb 5, 2018 •

edited

Loading

dekellum commented Feb 5, 2018 •

edited

Loading

dekellum Feb 9, 2018 •

edited

Loading

dekellum Feb 9, 2018 •

edited

Loading