Skip to content

Commit

Permalink
[mdoc] Fixes an issue with member and type deletion.
Browse files Browse the repository at this point in the history 
Specifically, when updating classic and unified assemblies, members
that were completely removed from one or both assemblies were not
being removed from the XML. This patch updates mdoc's `DeleteMember`
method to properly handle deletion in all scenarios (normal, classic, unified).

Of course, deletion will still not happen if one of two things is true:

- `--delete` is *not* passed into the `mdoc update` call and the member node
  contains existing documentation.
- `--preserve` *is* passed into the `mdoc update` invocation.

In both of those cases, the member will not be deleted, and a message stating
as such will be written as a warning to the output.
  • Loading branch information
@joelmartinez
joelmartinez committed Sep 1, 2015
1 parent 36e40b0 commit 09b97cb
Show file tree
Hide file tree
Showing 9 changed files with 461 additions and 26 deletions.
29 changes: 29 additions & 0 deletions mcs/tools/mdoc/Makefile
View file
Expand Up @@ -108,6 +108,22 @@ Test/DocTest-DropNS-classic.dll:
Test/DocTest-DropNS-unified.dll:
$(CSCOMPILE) $(TEST_CSCFLAGS) -debug -unsafe -target:library -out:$@ Test/DocTest-DropNS-unified.cs

Test/DocTest-DropNS-unified-deletetest.dll:
rm -f Test/DocTest-DropNS-unified-deletetest.dll
$(CSCOMPILE) $(TEST_CSCFLAGS) -debug -unsafe -target:library -out:$@ Test/DocTest-DropNS-unified.cs /define:DELETETEST

Test/DocTest-DropNS-unified-deletetest-V2.dll:
rm -f Test/DocTest-DropNS-unified-deletetest.dll
$(CSCOMPILE) $(TEST_CSCFLAGS) -debug -unsafe -target:library -out:Test/DocTest-DropNS-unified-deletetest.dll Test/DocTest-DropNS-unified.cs /define:DELETETEST,V2

Test/DocTest-DropNS-classic-deletetest.dll:
rm -f Test/DocTest-DropNS-classic-deletetest.dll
$(CSCOMPILE) $(TEST_CSCFLAGS) -debug -unsafe -target:library -out:$@ Test/DocTest-DropNS-classic.cs /define:DELETETEST

Test/DocTest-DropNS-classic-deletetest-V2.dll:
rm -f Test/DocTest-DropNS-classic-deletetest.dll
$(CSCOMPILE) $(TEST_CSCFLAGS) -debug -unsafe -target:library -out:Test/DocTest-DropNS-classic-deletetest.dll Test/DocTest-DropNS-classic.cs /define:DELETETEST,V2

Test/DocTest.dll:
$(CSCOMPILE) $(TEST_CSCFLAGS) -debug -unsafe -target:library -out:$@ Test/DocTest.cs

Expand Down Expand Up @@ -151,6 +167,18 @@ check-monodocer-dropns-classic: $(PROGRAM)
$(MAKE) update-monodocer-dropns-unified
diff --exclude=.svn -rup Test/en.expected-dropns-classic-v1 Test/en.actual

check-monodocer-dropns-delete: $(PROGRAM)
-rm -Rf Test/en.actual
$(MAKE) Test/DocTest-DropNS-classic-deletetest.dll
$(MONO) $(PROGRAM) update --delete --exceptions=all -o Test/en.actual Test/DocTest-DropNS-classic-deletetest.dll
$(MAKE) Test/DocTest-DropNS-unified-deletetest.dll
$(MONO) $(PROGRAM) update --delete --exceptions=all -o Test/en.actual Test/DocTest-DropNS-unified-deletetest.dll --dropns Test/DocTest-DropNS-unified-deletetest.dll=MyFramework
$(MAKE) Test/DocTest-DropNS-classic-deletetest-V2.dll
$(MONO) $(PROGRAM) update --delete --exceptions=all -o Test/en.actual Test/DocTest-DropNS-classic-deletetest.dll
$(MAKE) Test/DocTest-DropNS-unified-deletetest-V2.dll
$(MONO) $(PROGRAM) update --delete --exceptions=all -o Test/en.actual Test/DocTest-DropNS-unified-deletetest.dll --dropns Test/DocTest-DropNS-unified-deletetest.dll=MyFramework
diff --exclude=.dvn -rup Test/en.expected-dropns-delete Test/en.actual

check-monodocer-dropns-classic-withsecondary: $(PROGRAM)
# tests case where a secondary assembly is included with a --dropns parameter
-rm -Rf Test/en.actual
Expand Down Expand Up @@ -343,6 +371,7 @@ check-doc-tools-update: check-monodocer-since-update \
check-monodocer-update \
check-monodocer-dropns-classic \
check-monodocer-dropns-classic-withsecondary \
check-monodocer-dropns-delete \
check-monodocer-internal-interface \
check-monodocer-enumerations \
check-monodocer-delete-update \
Expand Down
114 changes: 88 additions & 26 deletions mcs/tools/mdoc/Mono.Documentation/monodocer.cs
View file
Expand Up @@ -187,6 +187,11 @@ public static bool HasDroppedNamespace(ModuleDefinition forModule)
return !string.IsNullOrWhiteSpace (droppedNamespace) && droppedAssemblies.Any(da => da == forModule.Name);
}

public static bool HasDroppedAnyNamespace ()
{
return !string.IsNullOrWhiteSpace (droppedNamespace);
}


static List<string> droppedAssemblies = new List<string>();

Expand Down Expand Up @@ -1012,11 +1017,40 @@ private void CleanupFiles (string dest, HashSet<string> goodfiles)
continue;
}

if (string.IsNullOrWhiteSpace (PreserveTag)) { // only do this if there was no -preserve
Action actuallyDelete = () => {
string newname = typefile.FullName + ".remove";
try { System.IO.File.Delete(newname); } catch (Exception) { }
try { typefile.MoveTo(newname); } catch (Exception) { }
Console.WriteLine("Class no longer present; file renamed: " + Path.Combine(nsdir.Name, typefile.Name));
try { System.IO.File.Delete (newname); } catch (Exception ex) { Warning ("Unable to delete existing file: {0}", newname); }
try { typefile.MoveTo (newname); } catch (Exception ex) { Warning ("Unable to rename to: {0}", newname); }
Console.WriteLine ("Class no longer present; file renamed: " + Path.Combine (nsdir.Name, typefile.Name));
};

if (string.IsNullOrWhiteSpace (PreserveTag)) { // only do this if there was not a -preserve
using (TextWriter writer = OpenWrite (typefile.FullName, FileMode.Truncate))
WriteXml (doc.DocumentElement, writer);

var unifiedAssemblyNode = doc.SelectSingleNode ("/Type/AssemblyInfo[@apistyle='unified']");
var classicAssemblyNode = doc.SelectSingleNode ("/Type/AssemblyInfo[@apistyle='classic']");
bool isUnifiedRun = HasDroppedAnyNamespace ();
bool isClassicOrNormalRun = !isUnifiedRun;
if (isClassicOrNormalRun) {
if (unifiedAssemblyNode != null) {
Warning ("*** this type is marked as unified, not deleting during this run: {0}", typefile.FullName);
// if truly removed from both assemblies, it will be removed fully during the unified run
continue;
} else {
// we should be safe to delete here because it was not marked as a unified assembly
actuallyDelete ();
}
}
if (isUnifiedRun) {
if (classicAssemblyNode != null) {
Warning ("*** this type is marked as classic, not deleting {0}", typefile.FullName);
continue;
} else {
// safe to delete because it wasn't marked as a classic assembly, so the type is gone in both.
actuallyDelete ();
}
}
}
}
}
Expand Down Expand Up @@ -1291,34 +1325,51 @@ void DeleteMember (string reason, string output, XmlNode member, MyXmlNodeList t
string format = output != null
? "{0}: File='{1}'; Signature='{4}'"
: "{0}: XPath='/Type[@FullName=\"{2}\"]/Members/Member[@MemberName=\"{3}\"]'; Signature='{4}'";
string signature = member.SelectSingleNode ("MemberSignature[@Language='C#']/@Value").Value;
Warning (format,
reason,
output,
member.OwnerDocument.DocumentElement.GetAttribute ("FullName"),
member.Attributes ["MemberName"].Value,
member.SelectSingleNode ("MemberSignature[@Language='C#']/@Value").Value);
if (!delete && MemberDocsHaveUserContent (member)) {
Warning ("Member deletions must be enabled with the --delete option.");
} else if (HasDroppedNamespace (type)) {
// if we're dropping the namespace, add the "classic style"
var existingAttribute = member.Attributes ["apistyle"];
if (existingAttribute != null) {
existingAttribute.Value = "classic";
} else {
// add the attribute and do not remove
XmlAttribute apistyleAttr = member.OwnerDocument.CreateAttribute ("apistyle");
signature);

apistyleAttr.Value = "classic";
// Identify all of the different states that could affect our decision to delete the member
bool shouldPreserve = !string.IsNullOrWhiteSpace (PreserveTag);
bool hasContent = MemberDocsHaveUserContent (member);
bool shouldDelete = !shouldPreserve && (delete || !hasContent);

member.Attributes.Append (apistyleAttr);
}
} else if (!HasDroppedNamespace (type) && member.Attributes ["apistyle"] != null && member.Attributes ["apistyle"].Value == "unified") {
// do nothing if there's an apistyle=new attribute and we haven't dropped the namespace
} else if (!string.IsNullOrWhiteSpace (PreserveTag)) {
// do nothing
} else {
bool unifiedRun = HasDroppedNamespace (type);

var classicAssemblyInfo = member.SelectSingleNode ("AssemblyInfo[@apistyle='classic']");
bool nodeIsClassic = classicAssemblyInfo != null;

Action actuallyDelete = () => {
todelete.Add (member);
deletions++;
};

if (!shouldDelete) {
// explicitly not deleting
string message = shouldPreserve ?
"Not deleting '{0}' due to --preserve." :
"Not deleting '{0}'; must be enabled with the --delete option";
Warning (message, signature);
} else if (unifiedRun && nodeIsClassic) {
// this is a unified run, and the member doesn't exist, but is marked as being in the classic assembly.
Warning ("Not removing '{0}' since it's still in the classic assembly.", signature);
} else if (unifiedRun && !nodeIsClassic) {
// unified run, and the node is not classic, which means it doesn't exist anywhere.
actuallyDelete ();
} else {
if (!nodeIsClassic) {
actuallyDelete ();
} else {
Warning ("Removing classic from '{0}' ... will be removed in the unified run if not present there.", signature);
member.RemoveApiStyle (ApiStyle.Classic);
if (classicAssemblyInfo != null) {
member.RemoveChild (classicAssemblyInfo);
}
}
}
}

Expand Down Expand Up @@ -2882,21 +2933,32 @@ enum ApiStyle {
static class DocUtils {

public static bool DoesNotHaveApiStyle(this XmlElement element, ApiStyle style) {
string styleString = style.ToString ().ToLower ();
string styleString = style.ToString ().ToLowerInvariant ();
string apistylevalue = element.GetAttribute ("apistyle");
return apistylevalue != styleString || string.IsNullOrWhiteSpace(apistylevalue);
}
public static bool HasApiStyle(this XmlElement element, ApiStyle style) {
string styleString = style.ToString ().ToLower ();
string styleString = style.ToString ().ToLowerInvariant ();
return element.GetAttribute ("apistyle") == styleString;
}
public static void AddApiStyle(this XmlElement element, ApiStyle style) {
string styleString = style.ToString ().ToLower ();
string styleString = style.ToString ().ToLowerInvariant ();
var existingValue = element.GetAttribute ("apistyle");
if (string.IsNullOrWhiteSpace (existingValue) || existingValue != styleString) {
element.SetAttribute ("apistyle", styleString);
}
}
public static void RemoveApiStyle (this XmlElement element, ApiStyle style)
{
element.RemoveAttribute (style.ToString ().ToLowerInvariant ());
}
public static void RemoveApiStyle (this XmlNode node, ApiStyle style)
{
var styleAttribute = node.Attributes ["apistyle"];
if (styleAttribute != null) {
node.Attributes.Remove (styleAttribute);
}
}

public static bool IsExplicitlyImplemented (MethodDefinition method)
{
Expand Down
20 changes: 20 additions & 0 deletions mcs/tools/mdoc/Test/DocTest-DropNS-classic.cs
View file
Expand Up @@ -4,5 +4,25 @@ public class MyClass {
public float Hello(int value) {
return 0.0f;
}
#if DELETETEST
public string InBoth {get;set;}
public string InBothClassic {get;set;}
public int InBothMagicType {get;set;}
#endif

#if DELETETEST && V2
public string AddedInV2 {get;set;}
public string AddedInV2Classic {get;set;}
#endif
#if DELETETEST && !V2
public string WillDeleteInV2 {get;set;}
public string WillDeleteInV2Classic {get;set;}
#endif
}

#if DELETETEST && !V2
public class WillDelete {
public string Name {get;set;}
}
#endif
}
25 changes: 25 additions & 0 deletions mcs/tools/mdoc/Test/DocTest-DropNS-unified.cs
View file
Expand Up @@ -4,5 +4,30 @@ public class MyClass {
public float Hello(int value) {
return 0.0f;
}
#if DELETETEST
public string InBoth {get;set;}
public string InBothUnified {get;set;}
public nint InBothMagicType {get;set;}
#endif

#if DELETETEST && V2
public string AddedInV2 {get;set;}
public string AddedInV2Unified {get;set;}
#endif
#if DELETETEST && !V2
public string WillDeleteInV2 {get;set;}
public string WillDeleteInV2Unified {get;set;}
#endif
}

#if DELETETEST
public struct nint {

}
#endif
#if DELETETEST && !V2
public class WillDelete {
public string Name {get;set;}
}
#endif
}

2 comments on commit 09b97cb

@akoeplinger
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joelmartinez seems like this causes an error when I run make check:

[...]
MONO_PATH="./../../class/lib/net_4_x:$MONO_PATH" /home/alexander/dev/mono/runtime/mono-wrapper  mdoc.exe update --delete --exceptions=all -o Test/en.actual Test/DocTest-DropNS-unified-deletetest.dll --dropns Test/DocTest-DropNS-unified-deletetest.dll=MyFramework
Updating: MyNamespace.MyClass
mdoc: Member Removed: File='Test/en.actual/MyFramework.MyNamespace/MyClass.xml'; Signature='public string AddedInV2Classic { get; set; }'
mdoc: Not removing 'public string AddedInV2Classic { get; set; }' since it's still in the classic assembly.
mdoc: Member Removed: File='Test/en.actual/MyFramework.MyNamespace/MyClass.xml'; Signature='public string InBothClassic { get; set; }'
mdoc: Not removing 'public string InBothClassic { get; set; }' since it's still in the classic assembly.
Updating: MyNamespace.nint
Members Added: 0, Members Deleted: 0
diff --exclude=.dvn -rup Test/en.expected-dropns-delete Test/en.actual
Only in Test/en.expected-dropns-delete/MyFramework.MyNamespace: WillDelete.xml.remove
Only in Test/en.actual: MyNamespace
make: *** [check-monodocer-dropns-delete] Error 1
alexander@ubuntu-desktop:~/dev/mono/mcs/tools/mdoc$

@akoeplinger
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, it looks like the new targets you added are in the wrong place, they shouldn't be in check-doc-tools-update but in check-doc-tools:

@@ -360,6 +366,11 @@ check-doc-tools: check-monodocer-since \
        check-monodocer-importslashdoc \
        check-monodocer \
        check-monodocer-delete \
+       check-monodocer-dropns-classic \
+       check-monodocer-dropns-classic-withsecondary \
+       check-monodocer-dropns-delete \
+       check-monodocer-internal-interface \
+       check-monodocer-enumerations \
        check-mdoc-export-html \
        check-mdoc-export-html-with-version \
        check-mdoc-export-msxdoc \
@@ -369,11 +380,6 @@ check-doc-tools-update: check-monodocer-since-update \
        check-monodocer-importecmadoc-update \
        check-monodocer-importslashdoc-update \
        check-monodocer-update \
-       check-monodocer-dropns-classic \
-       check-monodocer-dropns-classic-withsecondary \
-       check-monodocer-dropns-delete \
-       check-monodocer-internal-interface \
-       check-monodocer-enumerations \
        check-monodocer-delete-update \
        check-mdoc-export-html-update \
        check-mdoc-export-msxdoc-update \

That's likely why the error didn't show up in CI yet.

Please sign in to comment.