Statically accessible sheet can be referred to by its code name

[SystemsModelling]

RD 2.4
Looks unproblematic to me. This is in an addin referring to an active workbook.

sSurveyID = ActiveWorkbook.Worksheets("Surveys").Range("A2").Value

[retailcoder]

That's possibly a bug indeed - the inspection means to flag ThisWorkbook.Worksheets("sheetname") and ThisWorkbook.Sheets("sheetname"), where "sheetname" is a worksheet that exists at compile-time in ThisWorkbook.

The inspection found that there's a worksheet named Surveys in the host document -- or in any case, it did find a sheet named "Surveys" in one of the opened VBA projects...

Is the sheet in the add-in workbook? If not, I think the inspection is making a bad assumption, and that needs to be fixed. If yes, then the inspection is working as intended and that should be ThisWorkbook.Worksheets("Surveys"), and the inspection is suggesting to use a code name instead, e.g. SurveysSheet, which is more robust.

[SystemsModelling]

Yes, it has a 'surveys' sheet, and it was the active XLSM (not the .XLAM addin as I first reported) so indeed Activeworkbook would find such a sheet. I'll close this.

[retailcoder]

Something is off about this inspection though - the passing tests indicate that it's basically issuing results for exactly the opposite of when it should be doing that: this inspection produces 100% false positives, ...and 100% false negatives.

I tweaked it a bit last night to fix the false positives (adjusted the tests accorsingly), and got failing tests for the two situations it should be issuing a result:

• Unqualified Sheets or Worksheets member call in the ThisWorkbook module with a string literal argument referring to a sheet name that exists in ThisWorkbook at compile-time.
• Sheets or Worksheets member calls against the ThisWorkbook object, with a string literal argument referring to a sheet name that exists in ThisWorkbook at compile-time.

The problem is that Rubberduck is systematically resolving the member calls to Global.Sheets or Global.Worksheets, because the resolver isn't seeing the Workbook interface of the ThisWorkbook module (or the Worksheet interface of the e.g. Sheet1 module) - that's something we need to tap into the internal project type library to pick up - we have the API to do this, but it was never leveraged.

So until the resolver leverages the typelib API, this inspection is utterly broken.

[rkapka]

the passing tests indicate that it's basically issuing results for exactly the opposite of when it should be doing that

Huh? Are you sure? 😮
Here's an example test case:

[Test]
[Category("Inspections")]
public void SheetAccessedUsingString_ReturnsResult_AccessingUsingApplicationModule()
{
    const string inputCode =
@"Public Sub Foo()
    Application.Worksheets(""Sheet1"").Range(""A1"") = ""Foo""
    Application.Sheets(""Sheet1"").Range(""A1"") = ""Foo""
End Sub";

    using (var state = ArrangeParserAndParse(inputCode))
    {
        var inspection = new SheetAccessedUsingStringInspection(state);
        var inspectionResults = inspection.GetInspectionResults(CancellationToken.None);

        Assert.AreEqual(2, inspectionResults.Count());
    }
}

Looks right to me. We are getting an inspection result when Sheet1 is not accessed by its code name.

[MDoerner]

This test case seems to test the wrong thing, indeed.

Application.Worksheets basically resolves to Application.ActiveWorkbook.Worksheets, which is not the same as ThisWorkbook.Worksheets, in general. Using the the code name yields the same result as the latter. The active workbook might be an entirely different workbook to which there is no reference on compile time.

[rkapka]

Application.Worksheets basically resolves to Application.ActiveWorkbook.Worksheets, which is not the same as ThisWorkbook.Worksheets

If this is true then the inspection should not yield a result in such case. That being said, I think @retailcoder's opinion about testing the opposite is incorrect, at least to some extent.

[retailcoder]

This is testing the right thing - but it's also failing, and ignored/inconclusive:

Rubberduck/RubberduckTests/Inspections/SheetAccessedUsingStringInspectionTests.cs

Lines 16 to 34 in 0db534e

	[Test]
	[Ignore("See #4411")]
	[Category("Inspections")]
	public void SheetAccessedUsingString_ReturnsResult_AccessingUsingWorkbookModule()
	{
	const string inputCode =
	@"Public Sub Foo()
	ThisWorkbook.Worksheets(""Sheet1"").Range(""A1"") = ""Foo""
	ThisWorkbook.Sheets(""Sheet1"").Range(""A1"") = ""Foo""
	End Sub";
	using (var state = ArrangeParserAndParse(inputCode))
	{
	var inspection = new SheetAccessedUsingStringInspection(state);
	var inspectionResults = inspection.GetInspectionResults(CancellationToken.None);
	Assert.AreEqual(2, inspectionResults.Count());
	}
	}

Ignored useful tests: 1, false negatives: 1

---

This second test is wrong - the ActiveWorkbook may or may not be ThisWorkbook. This is testing exactly the opposite of what it should; results count should definitely be 0 in this case:

Rubberduck/RubberduckTests/Inspections/SheetAccessedUsingStringInspectionTests.cs

Lines 36 to 53 in 0db534e

	[Test]
	[Category("Inspections")]
	public void SheetAccessedUsingString_ReturnsResult_AccessingUsingApplicationModule()
	{
	const string inputCode =
	@"Public Sub Foo()
	Application.Worksheets(""Sheet1"").Range(""A1"") = ""Foo""
	Application.Sheets(""Sheet1"").Range(""A1"") = ""Foo""
	End Sub";
	using (var state = ArrangeParserAndParse(inputCode))
	{
	var inspection = new SheetAccessedUsingStringInspection(state);
	var inspectionResults = inspection.GetInspectionResults(CancellationToken.None);
	Assert.AreEqual(2, inspectionResults.Count());
	}
	}

False positives: 1

---

This third test is also wrong, same as above - unless the code is in the ThisWorkbook module (a missing useful scenario), expected results count should also be 0 here:

Rubberduck/RubberduckTests/Inspections/SheetAccessedUsingStringInspectionTests.cs

Lines 55 to 72 in 0db534e

	[Test]
	[Category("Inspections")]
	public void SheetAccessedUsingString_ReturnsResult_AccessingUsingGlobalModule()
	{
	const string inputCode =
	@"Public Sub Foo()
	Worksheets(""Sheet1"").Range(""A1"") = ""Foo""
	Sheets(""Sheet1"").Range(""A1"") = ""Foo""
	End Sub";
	using (var state = ArrangeParserAndParse(inputCode))
	{
	var inspection = new SheetAccessedUsingStringInspection(state);
	var inspectionResults = inspection.GetInspectionResults(CancellationToken.None);
	Assert.AreEqual(2, inspectionResults.Count());
	}
	}

False positives: 2

---

The fourth test is ensuring a Workbook dereferenced from the Workbooks collection doesn't issue any results - that's a good test to have... but it's also failing/inconclusive and ignored:

Rubberduck/RubberduckTests/Inspections/SheetAccessedUsingStringInspectionTests.cs

Lines 74 to 92 in 0db534e

	[Test]
	[Ignore("Ref #4329")]
	[Category("Inspections")]
	public void SheetAccessedUsingString_DoesNotReturnResult_AccessingUsingWorkbooksProperty()
	{
	const string inputCode =
	@"Public Sub Foo()
	Workbooks(""Foo"").Worksheets(""Sheet1"").Range(""A1"") = ""Foo""
	Workbooks(""Foo"").Sheets(""Sheet1"").Range(""A1"") = ""Foo""
	End Sub";
	using (var state = ArrangeParserAndParse(inputCode))
	{
	var inspection = new SheetAccessedUsingStringInspection(state);
	var inspectionResults = inspection.GetInspectionResults(CancellationToken.None);
	Assert.AreEqual(0, inspectionResults.Count());
	}
	}

Ignored useful tests: 2

---

The fifth test is passing and uses code that should indeed issue inspection results, if the sheet name existed in ThisWorkbook - this is a useful edge-case test:

Rubberduck/RubberduckTests/Inspections/SheetAccessedUsingStringInspectionTests.cs

Lines 94 to 111 in 0db534e

	[Test]
	[Category("Inspections")]
	public void SheetAccessedUsingString_DoesNotReturnResult_NoSheetWithGivenNameExists()
	{
	const string inputCode =
	@"Public Sub Foo()
	ThisWorkbook.Worksheets(""BadName"").Range(""A1"") = ""Foo""
	ThisWorkbook.Sheets(""BadName"").Range(""A1"") = ""Foo""
	End Sub";
	using (var state = ArrangeParserAndParse(inputCode))
	{
	var inspection = new SheetAccessedUsingStringInspection(state);
	var inspectionResults = inspection.GetInspectionResults(CancellationToken.None);
	Assert.AreEqual(0, inspectionResults.Count());
	}
	}

Passing edge-case tests: 1

---

The sixth test is also passing, and is a useful edge-case test.

Rubberduck/RubberduckTests/Inspections/SheetAccessedUsingStringInspectionTests.cs

Lines 113 to 131 in 0db534e

	[Test]
	[Category("Inspections")]
	public void SheetAccessedUsingString_DoesNotReturnResult_SheetWithGivenNameExistsInAnotherProject()
	{
	const string inputCode =
	@"Public Sub Foo()
	ThisWorkbook.Worksheets(""SheetFromOtherProject"").Range(""A1"") = ""Foo""
	ThisWorkbook.Sheets(""SheetFromOtherProject"").Range(""A1"") = ""Foo""
	End Sub";
	// Referenced project is created inside helper method
	using (var state = ArrangeParserAndParse(inputCode))
	{
	var inspection = new SheetAccessedUsingStringInspection(state);
	var inspectionResults = inspection.GetInspectionResults(CancellationToken.None);
	Assert.AreEqual(0, inspectionResults.Count());
	}
	}

Passing edge-case tests: 2

---

The seventh and last test is also passing, and mainly serves to document a limitation, i.e. the inspection won't evaluate non-literal expressions - I'd call that a useful edge-case test:

Rubberduck/RubberduckTests/Inspections/SheetAccessedUsingStringInspectionTests.cs

Lines 133 to 153 in 0db534e

	[Test]
	[Category("Inspections")]
	public void SheetAccessedUsingString_DoesNotReturnResult_AccessingUsingVariable()
	{
	const string inputCode =
	@"Public Sub Foo()
	Dim s As String
	s = ""Sheet1""
	ThisWorkbook.Worksheets(s).Range(""A1"") = ""Foo""
	ThisWorkbook.Sheets(s).Range(""A1"") = ""Foo""
	End Sub";
	using (var state = ArrangeParserAndParse(inputCode))
	{
	var inspection = new SheetAccessedUsingStringInspection(state);
	var inspectionResults = inspection.GetInspectionResults(CancellationToken.None);
	Assert.AreEqual(0, inspectionResults.Count());
	}
	}

Passing edge-case tests: 3

---

Recap:

• The most important tests that should be documenting the intended behavior, are being ignored and/or failing.
• Two tests are literally documenting that false positives are expected.
• The three remaining passing tests are useful, but test things around the inspection mechanics.

One problem is that setting up a mock ThisWorkbook document module that implements Excel.Worksheet isn't being done.

To clarify, this inspection should be issuing results when:

• A string literal matching an existing worksheet name is passed as an index argument to an unqualified Sheets or Worksheets member call in the ThisWorkbook module, OR:
• A string literal matching an existing worksheet name is passed as an index argument to a qualified Sheets or Worksheets member call against the ThisWorkbook object.

Anything else shouldn't be issuing results.

[retailcoder]

One problem is that setting up a mock ThisWorkbook document module that implements Excel.Worksheet isn't being done.

Not sure it can be done either. This might be a case where the best we can do is have a bunch of tests that ensure no false-negative situation arises, and two ignored tests document the "happy path" results-issuing code.